Skip to content

Conversation

ferdymercury
Copy link
Collaborator

Fixes #19850

@ferdymercury ferdymercury marked this pull request as ready for review September 10, 2025 13:03
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Will you take care of the backport to 6.36, or should I do it?

@ferdymercury
Copy link
Collaborator Author

Thanks a lot! Will you take care of the backport to 6.36, or should I do it?

I could do it. The question to me before is:

  • There are more ::getenv throughout the codebase without std:: prepended. Not sure if they should also be changed, for consistency or if that will break things on Windows or who knows what.
  • The user also reports a failure with cxxmodules after fixing this, (see same issue)

@guitargeek
Copy link
Contributor

Good points!

  • we should use std::getenv everywhere. The idea of moden C++ is to not use the C headers like stdlib.h, but instead use the standard library headers like cstdlib. See also this clang tidy check:
    https://clang.llvm.org/extra/clang-tidy/checks/modernize/deprecated-headers.html
    So it's probably a good opportunity to use std::getenv all the way 👍. Can we do it in this PR?
    This should also work on Windows, as the functions do the same thing.
  • I have seen that, that's a shame... Let's continue the discussion in that PR and ping our macOS expert @couet when we're stuck

See also:

Copy link

github-actions bot commented Sep 10, 2025

Test Results

    20 files      20 suites   3d 22h 54m 48s ⏱️
 3 663 tests  3 661 ✅   0 💤 2 ❌
71 547 runs  71 439 ✅ 106 💤 2 ❌

For more details on these failures, see this check.

Results for commit 0c1f275.

♻️ This comment has been updated with latest results.

@ferdymercury
Copy link
Collaborator Author

Can we do it in this PR?

Done as suggested

@ferdymercury ferdymercury force-pushed the getenv branch 5 times, most recently from a3e3816 to 61a328b Compare September 10, 2025 16:51
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Nice, thank you very much!

@guitargeek guitargeek merged commit 5a0ae56 into root-project:master Sep 10, 2025
21 of 25 checks passed
@ferdymercury ferdymercury deleted the getenv branch September 10, 2025 21:31
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.

Error: "no member named 'getenv' in the global namespace" during the compilation with libc++
3 participants