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

Compile otherlibs/ C stubs in two version for native and bytecode #11828

Merged
merged 1 commit into from Dec 23, 2022

Conversation

OlivierNicole
Copy link
Contributor

Currently, the libraries in otherlibs/ use a set of C files that are compiled and used to generate both a static library ($(CLIBNAME).$(A), where $(A) is a on Linux) and a dynamic one (dll$(CLIBNAME).$(SO)).

While working on ThreadSanitizer support, we found ourselves needing to use specific compilation flags for the native backend, and therefore to compile otherlibs libraries in two different versions, one for bytecode and one for native. This seems to be a natural enough need to propose a PR for it. @dra27 mentioned that he has a Windows-related use case for this, too.

@shindere
Copy link
Contributor

shindere commented Dec 20, 2022 via email

@OlivierNicole
Copy link
Contributor Author

  • For symmetry, I'd use byt as a prefix for the bytecode verison of the stub library but others may disagree here

i also prefer symmetry, but I mimicked what is already done for libthreads. I’m happy to do the symmetric solution if you prefer though.

@OlivierNicole
Copy link
Contributor Author

  • You could use a variable to define the C sources andthen compute your lists of objects fromthere, as is done in the Unix case, that would save some redundency

Done. I also squashed the commits, and fixed a typo that prevented make install from working.

@shindere
Copy link
Contributor

shindere commented Dec 21, 2022 via email

@OlivierNicole
Copy link
Contributor Author

  • For symmetry, I'd use byt as a prefix for the bytecode verison of the stub library but others may disagree here i also prefer symmetry, but I mimicked what is already done for libthreads. I’m happy to do the symmetric solution if you prefer though.

I'd tend to do the symmetric one, including htreads. Let's try and fallback to non-symmetry only if somebody complains. :-)

OK, done.

@shindere
Copy link
Contributor

shindere commented Dec 23, 2022 via email

@shindere shindere merged commit 4935f7b into ocaml:trunk Dec 23, 2022
@OlivierNicole OlivierNicole deleted the distinct_n_b_objects branch December 23, 2022 09:58
@xavierleroy
Copy link
Contributor

The build fails at installation time on bytecode-only systems:

make[1]: Entering directory '/builds/workspace/main/flambda/false/label/ocaml-linux-32/otherlibs/runtime_events'
if test -f dllcamlruntime_eventsbyt.so; then \
  /usr/bin/install -c -p \
    dllcamlruntime_eventsbyt.so "/home/ci/ocaml-tmp-install-3276/lib/ocaml/stublibs"; \
fi
if test -f dllcamlruntime_eventsnat.so; then \
  /usr/bin/install -c -p \
    dllcamlruntime_eventsnat.so "/home/ci/ocaml-tmp-install-3276/lib/ocaml/stublibs"; \
fi
/usr/bin/install -c -p -m 644 libcamlruntime_eventsbyt.a "/home/ci/ocaml-tmp-install-3276/lib/ocaml/"
/usr/bin/install -c -p -m 644 libcamlruntime_eventsnat.a "/home/ci/ocaml-tmp-install-3276/lib/ocaml/"
/usr/bin/install: cannot stat 'libcamlruntime_eventsnat.a': No such file or directory
../Makefile.otherlibs.common:105: recipe for target 'install' failed

OlivierNicole added a commit to OlivierNicole/ocaml that referenced this pull request Dec 29, 2022
OlivierNicole added a commit to OlivierNicole/ocaml that referenced this pull request Dec 29, 2022
@OlivierNicole
Copy link
Contributor Author

Sorry for forgetting to check the bytecode-only build. I propose a fix in #11842.

xavierleroy pushed a commit that referenced this pull request Dec 30, 2022
This moves the relevant install steps from the install to the installopt target.
ilankri added a commit to ilankri/geneweb that referenced this pull request Dec 25, 2023
ilankri added a commit to ilankri/geneweb that referenced this pull request Dec 26, 2023
ilankri added a commit to ilankri/geneweb that referenced this pull request Jan 7, 2024
canonici pushed a commit to geneweb/geneweb that referenced this pull request Feb 1, 2024
canonici pushed a commit to geneweb/geneweb that referenced this pull request Feb 1, 2024
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.

None yet

3 participants