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

Export SUPPORTS_SHARED_LIBRARIES in ocamlc -config #1691

Merged
merged 1 commit into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@dra27
Copy link
Contributor

commented Apr 2, 2018

Exposes SUPPORTS_SHARED_LIBRARIES from Makefile.config as shared_libraries in ocamlc -config

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

I support the feature and believe the implementation is correct. Do we get five minutes to discuss whether supports_shared_libraries isn't a more descriptive name (in term of suggesting booleanness) than shared_libraries?

(supports_shared_libraries is what ocamlbuild's ocamlbuild_config.ml uses, but I think there the idea was just to directly reuse the upstream name rather than any conscious naming decision.)

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2018

I um'd and ah'd over the name (it has two names in our code base too - it's different in s.h!). I'm not attached to the name at all, although other booleans (e.g. windows_unicode, flamda) have similarly non-Boolean names.

I'm in no particular rush for this to be merged, so happy to wait for other additional (!) opinions on suitable naming.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

I tend to think it would be better to keep this patch consistent as regards naming, so the config dump should be "supports_shared_libraries". @dra27 Can you make this change and then merge?

@gasche

gasche approved these changes Apr 10, 2018

Copy link
Member

left a comment

(independently of the naming discussion, here is a magical seal of approval.)

@dra27 dra27 force-pushed the dra27:support-dynamic-linking-config branch from ff9f818 to 4aabef3 Sep 11, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

@mshinwell - (finally) done. I'll merge when CI passes.

@dra27 dra27 merged commit 6fab940 into ocaml:trunk Sep 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dra27 dra27 deleted the dra27:support-dynamic-linking-config branch Sep 13, 2018

@dra27 dra27 referenced this pull request Nov 23, 2018

Merged

Fix -no-shared-libs support #2160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.