Skip to content

Fix environ define for Apple #175

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

Closed
wants to merge 1 commit into from
Closed

Conversation

barracuda156
Copy link
Contributor

Fix the definition for macOS.

@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (fa7fa30) 98.22% compared to head (a4041d7) 98.22%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #175   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         140      140           
  Lines       11887    11887           
=======================================
  Hits        11676    11676           
  Misses        211      211           
Files Changed Coverage Δ
src/Corrade/Utility/Arguments.cpp 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mosra
Copy link
Owner

mosra commented Aug 6, 2023

Hi, thanks for the PR!

Can you explain why this change is needed? Because, as far as I could see, the original worked as well -- compiled and passed tests. For comparison, here's the ArgumentsTest output for a build on current master:

72: Starting Corrade::Utility::Test::ArgumentsTest with 96 test cases...
72: Environment variables found: 60
72: One environment variable: RUBY_ENGINE=ruby
72:     OK [01] environment()
72:     OK [02] environmentUtf8()
72:     OK [03] copy()
72:     OK [04] move()
72:     OK [05] helpArgumentsOnly()
72:     ...

And here's a build of your PR:

72: Starting Corrade::Utility::Test::ArgumentsTest with 96 test cases...
72: Environment variables found: 64
72: One environment variable: PWD=/Users/distiller/project/build
72:     OK [01] environment()
72:     OK [02] environmentUtf8()
72:     OK [03] copy()
72:     OK [04] move()
72:     OK [05] helpArgumentsOnly()
72:     ...

In both cases it populates the environment, and env vars explicitly set before the test (ARGUMENTSTEST_UNICODE in this case) are contained in that list.

Is the environ variable causing warnings or errors in some cases? Is it not working on some macOS versions? Or what's the reason? I'm not opposing this change, I just want to understand it.

@barracuda156
Copy link
Contributor Author

@mosra Thank you for responding! Reason is that while on the new macOS those are apparently equivalent, on older ones that environ declaration breaks the build, which fails at linking due to missing environ symbol.
(It is a known issue, we had to fix it for a number of ports in Macports.)

@mosra
Copy link
Owner

mosra commented Aug 6, 2023

Hm, I found this, which I suppose is what your change is based on: https://epics.anl.gov/tech-talk/2009/msg00299.php

Assuming the original code caused a build failure on your side, can you give me more details about the scenario in which it did? So I can add that to the test suite, or at least put an explanatory comment to the code.

Thank you!

@mosra
Copy link
Owner

mosra commented Aug 6, 2023

Hah, commented at the same time. Which macOS versions are affected? Just for documentation purposes.

Unfortunately I'm not able to test anything older than 11 on the CIs to have it caught in an automated way...

@barracuda156
Copy link
Contributor Author

barracuda156 commented Aug 6, 2023

Hah, commented at the same time. Which macOS versions are affected? Just for documentation purposes.

Unfortunately I'm not able to test anything older than 11 on the CIs to have it caught in an automated way...

I am not really sure re all macOS versions that fail due to this, to be honest. I can say for the fact that 10.6 is affected, and I think it is relevant for some other systems.
Here is some relevant info: https://www.gnu.org/software/gnulib/manual/html_node/environ.html
Plus: https://www.mail-archive.com/bug-gnulib@gnu.org/msg09272.html
Looks like it is not working after 10.4 (and I do not really like Tiger and seldom test anything there, so cannot confirm). But apparently was returned to macOS or fixed somewhere later.

@mosra
Copy link
Owner

mosra commented Aug 6, 2023

I did some digging, found https://ports.macports.org/all_builds/?port_name=corrade and there it seems to be an issue before macOS 10.8. I reworked your patch a bit (didn't like the #define) and merged as 3465b5e.

10.8 and 10.9 builds then fail on __rdtsc not being defined, not sure what's the fix for that 😅 In general I already removed support for Clang versions before 6 (so anything older than Xcode 10, and anything before macOS 10.15) as from my side it's basically impossible to reliably test for these. Thus I can't guarantee that the project will work there going forward -- it'd rely on "community fixes" like the one you made here (sorry).

@mosra mosra closed this Aug 6, 2023
@barracuda156
Copy link
Contributor Author

@mosra Thank you! Community fixes are perfectly fine. Personally I do not care about Clangs at all (in Macports I have to, cannot just ignore them); GCC works fine both on PowerPC and Intel.

I can confirm that it does build on 10.6 with the modern GCC :)

@mosra mosra added this to the 2023.0a milestone Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants