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.

Sign up for GitHub

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

Jump to bottom

Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 #10962

Merged
merged 3 commits into from
Oct 6, 2024
Merged

Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 #10962

merged 3 commits into from
Oct 6, 2024

Conversation

kit-ty-kate
Copy link
Member

Fixes #10886

@kit-ty-kate kit-ty-kate mentioned this pull request Sep 28, 2024
Closed
@Leonidas-from-XIV
Copy link
Collaborator

Could you please add a CHANGES entry? This seems to be an interesting enough change for users to have it documented.

@kit-ty-kate
Copy link
Member Author

Could you please add a CHANGES entry? This seems to be an interesting enough change for users to have it documented.

done

@rgrinberg
Copy link
Member

Can you also add a test that demonstrates the change in behavior?

rgrinberg
rgrinberg reviewed Oct 1, 2024
View reviewed changes
src/dune_rules/cxx_flags.mli
@@ -3,10 +3,6 @@

open Import

type phase =
| Compile
| Link
Copy link
Member

Choose a reason for hiding this comment

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

Is this refactoring essential to the fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

that type isn't used anywhere anymore and the removal of its use is essential to the fix (get_compile_flags is the only one that need the extra argument and places where get_link_flags is called do not have any way of getting that value, plus that would be wasteful)

Copy link
Member

Choose a reason for hiding this comment

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

You could have also just modified the type to be:

type phase =
  | Compile of Ocaml_config.t
  | Link

That would be a bit closer the old API and preserve some of the call sites.

It's not essential in this case as the refactoring is trivial, but in general I would like fixes and refactors to be as separate as possible.

@kit-ty-kate
Copy link
Member Author

Can you also add a test that demonstrates the change in behavior?

done

@kit-ty-kate
Copy link
Member Author

Are there any chance this could be merged in a patch release or something during this OCaml 5.3 alpha release cycle in the next couple of weeks? This would be really really appreciated

kit-ty-kate and others added 3 commits October 6, 2024 11:07
@kit-ty-kate @rgrinberg
Call the C++ compiler with -std=c++11 when using OCaml >= 5.0
b952d9a 
Signed-off-by: Kate <kit-ty-kate@outlook.com>
@rgrinberg
_
2060082 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
_
081269c 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg
Copy link
Member

Thanks. I reverted the refactorings and also changed the check to use Ocaml.Version.t as we do elsewhere.

@rgrinberg rgrinberg merged commit 250f0b1 into ocaml:main Oct 6, 2024
27 checks passed
This was referenced Oct 10, 2024
Merged
Open
maiste pushed a commit to maiste/dune that referenced this pull request Oct 29, 2024
@kit-ty-kate @maiste
Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 (ocaml#…
0261c00 
…10962)

* Call the C++ compiler with -std=c++11 when using OCaml >= 5.0
maiste pushed a commit to maiste/dune that referenced this pull request Oct 29, 2024
@kit-ty-kate @maiste
Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 (ocaml#…
f19d564 
…10962)

* Call the C++ compiler with -std=c++11 when using OCaml >= 5.0

Signed-off-by: Etienne Marais <dev@maiste.fr>
This was referenced Oct 29, 2024
Closed
Closed
Merged
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Oct 30, 2024
Merged
maiste added a commit that referenced this pull request Oct 30, 2024
@maiste
[3.16] backport: #11009 #10962 (#11051)
6f61ed5 
* Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 (#10962)
* Call the C++ compiler with -std=c++11 when using OCaml >= 5.0
* Fix the compilation of OCaml 5 programs on Cygwin (#11009)
strdup is not part of the C standard library but is part of POSIX.
By default GCC and Clang both use the GNU variant of the C standard,
so we are using strdup here to ensure users have access to it by default.
If -std=c++11 is used instead of -std=gnu++11 this would fail to compile
on non-POSIX platforms such as Cygwin.
* test: backport missing test file
---------
Signed-off-by: Etienne Marais <dev@maiste.fr>
Signed-off-by: Kate <kit-ty-kate@outlook.com>
Co-authored-by: Kate <kit.ty.kate@disroot.org>
Co-authored-by: Kate <kit-ty-kate@outlook.com>
maiste added a commit to maiste/opam-repository that referenced this pull request Oct 30, 2024
@maiste
[new release] dune (15 packages) (3.16.1)
ed800ba 
CHANGES:

### Fixed

- Call the C++ compiler with `-std=c++11` when using OCaml >= 5.0
  (ocaml/dune#10962, @kit-ty-kate)
@maiste maiste mentioned this pull request Oct 30, 2024
Merged
anmonteiro pushed a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
@kit-ty-kate @anmonteiro
Call the C++ compiler with -std=c++11 when using OCaml >= 5.0 (ocaml#…
ffd89a0 
…10962)

* Call the C++ compiler with -std=c++11 when using OCaml >= 5.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing -std=c++11 when compiling C++ files
3 participants
@kit-ty-kate @Leonidas-from-XIV @rgrinberg