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(stdlib): pass flags when building stdlib.ml #7241

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Mar 8, 2023

Reported by @gretay-js.

This ensures that when building stdlib.ml (the main module of a library with (stdlib)), flags set in the corresponding stanza (library) are correctly passed.

@emillon emillon requested a review from rgrinberg March 8, 2023 14:37
@emillon
Copy link
Collaborator Author

emillon commented Mar 8, 2023

I'm not sure how to add that to the test suite. To do that locally, I added (ocamlopt_flags :standard -alert +sample-alert) to the test in stdlib-compilation.t and checked that it's passed in the log, but that's not great.
Also @rgrinberg I'm not too sure how related this fix is to the comment just above so maybe there's a better fix. Thanks!

@anmonteiro
Copy link
Collaborator

@emillon I recently added some stdlib tests in #7139, perhaps that could be helpful.

src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/compilation_context.ml Outdated Show resolved Hide resolved
src/dune_rules/module_compilation.ml Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator Author

emillon commented Mar 9, 2023

Thanks. I added a test and implemented your suggestions. I didn't realize it was coming from the same value.

@emillon emillon requested a review from rgrinberg March 9, 2023 09:53
@rgrinberg rgrinberg added this to the 3.8.0 milestone Mar 13, 2023
Reported by @gretay-js.

This ensures that when building `stdlib.ml` (the main module of a
library with `(stdlib)`), flags set in the corresponding stanza
`(library)` are correctly passed.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon emillon merged commit 8621946 into ocaml:main Mar 14, 2023
@emillon emillon deleted the stdlib-flags branch March 14, 2023 09:08
emillon added a commit to emillon/dune that referenced this pull request Mar 31, 2023
Reported by @gretay-js.

This ensures that when building `stdlib.ml` (the main module of a
library with `(stdlib)`), flags set in the corresponding stanza
`(library)` are correctly passed.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Apr 3, 2023
Reported by @gretay-js.

This ensures that when building `stdlib.ml` (the main module of a
library with `(stdlib)`), flags set in the corresponding stanza
`(library)` are correctly passed.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Apr 4, 2023
Reported by @gretay-js.

This ensures that when building `stdlib.ml` (the main module of a
library with `(stdlib)`), flags set in the corresponding stanza
`(library)` are correctly passed.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/opam-repository that referenced this pull request Apr 4, 2023
…, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.7.1)

CHANGES:

- Fix segfault on MacOS when dune was being shutdown while in watch mode.
  (ocaml/dune#7312, fixes ocaml/dune#6151, @gridbugs, @emillon)

- Fix preludes not being recorded as dependencies in the `(mdx)` stanza (ocaml/dune#7109,
  fixes ocaml/dune#7077, @emillon).

- Pass correct flags when compiling `stdlib.ml`. (ocaml/dune#7241, @emillon)

- Handle "Too many links" errors when using Dune cache on Windows.  The fix in
  3.7.0 for this same issue was not effective due to a typo. (ocaml/dune#7472, @nojb)
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