Skip to content
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

Fix inability to find support/{platform,dirpath}.h from julia_fasttls.h. #41308

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jun 21, 2021

During PackageCompiler.jl system image creation under Julia 1.7, gcc complains that it cannot find support/{platform,dirpath}.h included from julia_fasttls.h (while looking in usr/include/julia). My (potentially incorrect) working understanding is that all of julia_fasttls.h and {platform,dirpath}.h end up directly in usr/include/julia --- i.e. the support/ qualification of support/{platform,dirpath}.h effectively gets flattened away when {platform,dirpath}.h are copied from src/support/{platform,dirpath}.h to usr/include/julia. This patch drops the support/ qualification in the include in julia_fasttls.h, which locally seems to make everything happy (though I imagine this may not be the best or even a generally correct approach). Best! :)

@Sacha0 Sacha0 added bugfix This change fixes an existing bug backport 1.7 labels Jun 21, 2021
@Sacha0 Sacha0 requested a review from staticfloat June 21, 2021 22:17
@staticfloat
Copy link
Sponsor Member

This only works because we add -I$(JULIAHOME)/src/support to our compiler invocations. I hate different build/install pathing; I'd much rather we install our headers as ${prefix}/include/support/platform.h, for instance.

That being said, I'm okay with this, how about you @vtjnash?

@Sacha0
Copy link
Member Author

Sacha0 commented Jun 22, 2021

This only works because we add -I$(JULIAHOME)/src/support to our compiler invocations.

Indeed :).

I hate different build/install pathing; I'd much rather we install our headers as ${prefix}/include/support/platform.h, for instance.

FWIW I share your preference. Happy to go with whichever you and other folks collectively prefer :).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 22, 2021

We probably should restructure our headers so that the public ones (this and julia.h) are the only visible ones, and the rest are tucked away somewhere, but this seems reasonable for now

@Sacha0
Copy link
Member Author

Sacha0 commented Jun 22, 2021

Cheers, it sounds like we would all prefer to restructure the headers at some point, but this patch does the trick for now. That being the case, I'll merge this patch to get things working pending some future restructuring.

Thanks for reviewing gentlemen! :)

@Sacha0 Sacha0 merged commit e4b2411 into master Jun 22, 2021
@Sacha0 Sacha0 deleted the ftls-support-includes branch June 22, 2021 22:01
fingolfin added a commit to fingolfin/Yggdrasil that referenced this pull request Jul 3, 2021
Taken from JuliaLang/julia#41308, this is
necessary in order to actually build C code against libjulia.
fingolfin added a commit to fingolfin/Yggdrasil that referenced this pull request Jul 3, 2021
Taken from JuliaLang/julia#41308, this is
necessary in order to actually build C code against libjulia.
giordano pushed a commit to JuliaPackaging/Yggdrasil that referenced this pull request Jul 3, 2021
Taken from JuliaLang/julia#41308, this is
necessary in order to actually build C code against libjulia.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants