Skip to content

ports/esp32/Makefile: Fix path expansion for ESPIDF_DRIVER_O #4919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Jul 11, 2019

It was using subst to s/.c/.o/ which changed .c anywhere in the path. (I have ESPIDF set to /home/jimmo/src/github.com/espressif/esp-idf, resulting in github.oom)

@dpgeorge
Copy link
Member

See #4874 for where this was added, in particular the discussion there about trying to fit this on one line. I guess it's not possible to do that?

It was using subst to s/.c/.o/ which changed .c anywhere in the path. (I have ESPIDF set to `/home/jimmo/src/github.com/espressif/esp-idf`, resulting in github.oom)
@jimmo
Copy link
Member Author

jimmo commented Jul 11, 2019

I spoke to @nevercast and apparently there were issues in their build using patsubst (although it works fine for me).

ESPIDF_DRIVER_O = $(patsubst %.c,%.o,$(wildcard $(ESPCOMP)/driver/*.c))

He's going to get back to me with more details.

@nevercast
Copy link
Contributor

I was sick last week and didn't get around to checking on this, I'll get to it this week. Sorry about the delay.

@jimmo jimmo force-pushed the esp32-espidf-driver-path branch from 585a1c6 to 9a69ef3 Compare July 19, 2019 07:23
@jimmo
Copy link
Member Author

jimmo commented Jul 19, 2019

I've updated to use patsubst, as far as I understand this is exactly what patsubst is for, and I can verify that it correctly generates the set of .o files corresponding to $(ESPCOMP)/driver/*.c.

@nevercast
Copy link
Contributor

I've spoken with @jimmo in Slack and while I haven't found the time to test this personally, I'm happy for the change. I have a build server at work that will test this commit against my own codebase as soon as it lands in #master so I'm not too worried.

@dpgeorge
Copy link
Member

I checked locally and it builds fine, builds the same way as before this patch. Also Travis is green.

Merged in 331c224

@dpgeorge dpgeorge closed this Jul 20, 2019
@nevercast
Copy link
Contributor

Jenkins passed at my end too 🎉

@dpgeorge
Copy link
Member

Great!

@dpgeorge
Copy link
Member

See #4935 for a follow up.

tannewt added a commit to tannewt/circuitpython that referenced this pull request Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants