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

Install only headers required by julia.h #17745

Closed
wants to merge 1 commit into from
Closed

Install only headers required by julia.h #17745

wants to merge 1 commit into from

Conversation

petercolberg
Copy link
Contributor

Closes #17657

src/support/uv-threadpool.h \
src/support/uv-unix.h \
src/support/uv-version.h \
src/support/uv.h
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these uv*.h aren't in src/support, they're done on a different line

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

I think this would have less chance of going stale if we filtered out the specific ones we know we don't need. Or move them to a different location like src/support/include

@petercolberg
Copy link
Contributor Author

I was thinking of actually testing the installed libjulia by building the embedding example:

cc -I/usr/include/julia -o embedding embedding.c -ldl -lrt -lpthread -lopenlibm -ljulia      
/tmp/cc660rPO.o: In function `main':
embedding.c:(.text+0x1c6): undefined reference to `jl_tls_states'
embedding.c:(.text+0x1e9): undefined reference to `jl_tls_states'
embedding.c:(.text+0x2fe): undefined reference to `jl_tls_states'
embedding.c:(.text+0x309): undefined reference to `jl_tls_states'
embedding.c:(.text+0x31a): undefined reference to `jl_tls_states'
collect2: error: ld returned 1 exit status

Any idea why the linker cannot find jl_tls_states?

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

yeah #8757 has been open for a while. I think you need a -DJULIA_ENABLE_THREADING

@petercolberg
Copy link
Contributor Author

Now the example compiles but fails to run:

./embedding 
ERROR: system image file "/home/peter/git/github.com/JuliaLang/julia/examples/../lib/x86_64-linux-gnu/julia/v0.5/sys.so" not found

Which variable sets the path to the system image?

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

JULIA_HOME, are you setting that as an environment variable? If so you're setting it to the wrong location, or probably shouldn't be setting it at all.

@petercolberg
Copy link
Contributor Author

It seems to work now:

JULIA_HOME=/usr ./embedding 
1.4142135623730951
sqrt(2.0) in C: 1.414214e+00
sqrt(2.0) in C: 1.414214e+00
x = [9.000000e+00 8.000000e+00 7.000000e+00 6.000000e+00 5.000000e+00 4.000000e+00 3.000000e+00 2.000000e+00 1.000000e+00 0.000000e+00 ]
my_func(5.0) = 10.000000
UndefVarError(:this_function_does_not_exist)

For the purpose of testing this is fine, but why is JULIA_HOME not set in libjulia.so?

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

See

jl_options.julia_home = getenv("JULIA_HOME");
, there's a getenv to allow you to point libjulia to use a different system image path if you don't have the command-line executable available - so exactly in the embedding case. A lot of people do seem to mistakenly set that variable to the wrong thing and Julia fails to start because of it, so we should document that better or pick a more specific name.

@petercolberg
Copy link
Contributor Author

Have you thought about an include dir for public headers, separate from src and src/support?

@tkelman
Copy link
Contributor

tkelman commented Aug 1, 2016

The install tree should look like that, but the source tree doesn't necessarily have to.

@petercolberg
Copy link
Contributor Author

Indeed. It is suggestion to make the developer aware that these are public headers.

@yuyichao
Copy link
Contributor

yuyichao commented Aug 2, 2016

Do note that headers required by julia.h != public headers / public APIs. There are functions defined in those headers that are intentionally hidden yet the types defined in the same header are needed to define the ABI.

@kshyatt kshyatt added the building Build system, or building Julia or its dependencies label Aug 2, 2016
@@ -324,6 +324,25 @@ define stringreplace
$(build_depsbindir)/stringreplace $$(strings -t x - $1 | grep '$2' | awk '{print $$1;}') '$3' 255 "$(call cygpath_w,$1)"
endef

JL_HEADERS := \
Copy link
Contributor

@tkelman tkelman Aug 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My prior feedback is now collapsed at #17745 (comment) still applies, I don't like this form where JL_HEADERS needs to be kept up to date. A shorter exclusions list of things that are currently included in src/support/*.h but aren't needed would be more maintainable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installs unneeded headers to <prefix>/include/julia?
5 participants