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 variable CC content + add test #3695

Merged
merged 2 commits into from
Aug 13, 2020
Merged

Fix variable CC content + add test #3695

merged 2 commits into from
Aug 13, 2020

Conversation

voodoos
Copy link
Collaborator

@voodoos voodoos commented Aug 12, 2020

History

  • Since Directly call the C compiler  #3565 ocaml_cflags were removed from the :standard set of flags to prevent flags duplication as these are always prepended to the C compiler arguments. However this is not a backward compatible change: an example of that is that it changes the content of variable %{cc}.
  • I proposed [Fix] Restore variable %{cc} content (changed by #3565 ) #3686 to restore compatibility but it was not exactly the same either: the new CC content would not be overwritten by cflags from an env stanza which do not include :standard.

This PR

  • This PR takes a much simpler approach: adding back the flags in :standard (which will provoke duplication again).
  • I also added tests illustrating the interactions between the CC variable and cflags defined in Env stanzas.

Future work

An RFC to propose a streamlining and maybe redefine the role of the CC variable.

@voodoos voodoos added this to the 2.7 milestone Aug 12, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good!

Nicely written test BTW, was nice and easy to read :)

@voodoos voodoos changed the title Cc test Fix variable CC content + add test Aug 12, 2020
@rgrinberg rgrinberg merged commit d935859 into ocaml:master Aug 13, 2020
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.

2 participants