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 ParseFlags so -stdlib flag goes in CXXFLAGS #4520

Closed
wants to merge 1 commit into from

Conversation

ryandesign
Copy link

@ryandesign ryandesign commented May 5, 2024

gpsd 3.25 failed to build with recent clang if -stdlib=libc++ was passed by the user in CXXFLAGS because gpsd used MergeFlags (which uses ParseFlags) to move the flags where SCons thinks they belong, and because SCons did not know where -stdlib goes it put it in CCFLAGS, which was then passed to the C compiler during a test, and clang emitted a warning that the -stdlib flag was unknown, and the test had requested that warnings be turned into errors with -Werror, which caused the test to get the wrong result which caused compilation to fail later.

See: https://gitlab.com/gpsd/gpsd/-/issues/279

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

gpsd 3.25 failed to build with recent clang if -stdlib=libc++ was passed
by the user in CXXFLAGS because gpsd used MergeFlags (which uses
ParseFlags) to move the flags where SCons thinks they belong, and
because SCons did not know where -stdlib goes it put it in CCFLAGS,
which was then passed to the C compiler during a test, and clang emitted
a warning that the -stdlib flag was unknown, and the test had requested
that warnings be turned into errors with -Werror, which caused the test
to get the wrong result which caused compilation to fail later.

See: https://gitlab.com/gpsd/gpsd/-/issues/279
@mwichmann
Copy link
Collaborator

Yes, unfortunately ParseFlags depends on being able to recognize each option, so it's down to people figuring out something is needed and how to handle it. Looks reasonable to me.

Will need to work up changleog entries and preferably add a testcase (most of these are touched either in SCons/EnvironmentTests.py or in test/ParseConfig.py).

@bdbaddog
Copy link
Contributor

bdbaddog commented May 5, 2024

@ryandesign - Yup as @mwichmann , please add a blurb to CHANGES.txt and RELEASE.txt, and a test and we'll get this merged.

@ryandesign
Copy link
Author

I'm not planning any further changes to this PR right now but others are welcome to add the missing pieces.

mwichmann added a commit to mwichmann/scons that referenced this pull request May 9, 2024
This is the completion of PR SCons#4520

Signed-off-by: Mats Wichmann <[email protected]>
@mwichmann
Copy link
Collaborator

Closing this in favor of #4521 which should complete the process (it was easier to spin up a new PR with small changes to four additional files than have to modify the existing one)

@mwichmann mwichmann closed this May 9, 2024
@ryandesign ryandesign deleted the patch-1 branch July 16, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants