Skip to content

Conversation

@barracuda156
Copy link
Contributor

@barracuda156 barracuda156 commented Feb 19, 2024

Recent 23112e9 commit has broken duckdb build on macOS < 10.7.
Add a fallback to fix it.
As an example of a similar code see get_ppid_of.c in gettext’s libtextstyle.

Recent commit has broken duckdb build on macOS < 10.7: duckdb@23112e9
Add a fallback to fix it.
See also get_ppid_of.c in gettext’s libtextstyle.
@github-actions github-actions bot marked this pull request as draft February 19, 2024 23:17
@Mytherin
Copy link
Collaborator

Mytherin commented Feb 20, 2024

Thanks for the PR! Hardcoding 128 as the byte size of this struct seems error-prone and likely to cause additional issues. Could we perhaps instead do a return string() and disable the extended error message in this situation?

@barracuda156
Copy link
Contributor Author

barracuda156 commented Feb 20, 2024

@Mytherin If you or someone is sure that gonna work, we could. gnulib does use 128 there for some reason: https://github.com/coreutils/gnulib/blob/37fa6373a3fcac65ab3500ff65868650f9782cba/lib/get_ppid_of.c#L240-L275
Specifically in this line: https://github.com/coreutils/gnulib/blob/37fa6373a3fcac65ab3500ff65868650f9782cba/lib/get_ppid_of.c#L269

Looks like it was originally introduced that way: coreutils/gnulib@015fe7c
And then modified a bit to support < 10.5: coreutils/gnulib@119622a (however hardcoding to 128 remained).

P. S. Provided this is correct for specific macOS versions (< 10.7), it is safe – or can be made so by extra macros specifying macOS versions explicitly. As long as the fallback sits inside, it cannot affect anything else (if that is your concern).
I cannot really make a judgement whether gnulib upstream used the optimal code – I can only say it does fix the build on my system.

@Mytherin
Copy link
Collaborator

That's fair - but since we are not running tests on older MacOS versions I would be more comfortable just returning string() - this is not a critical component of the system, it just returns a more detailed error message which could be skipped on older systems.

@barracuda156
Copy link
Contributor Author

That's fair - but since we are not running tests on older MacOS versions I would be more comfortable just returning string() - this is not a critical component of the system, it just returns a more detailed error message which could be skipped on older systems.

@Mytherin Okay, sure, let’s do that. Could you either add a commit here or make a code suggestion for me to commit?

@Mytherin Mytherin marked this pull request as ready for review February 20, 2024 10:19
@Mytherin
Copy link
Collaborator

Done - could you check if this resolves the issue for you on the older MacOS box? I don't have access to one.

@Mytherin Mytherin merged commit b22e8b7 into duckdb:main Feb 20, 2024
@barracuda156 barracuda156 deleted the fix_macos branch February 20, 2024 14:26
@barracuda156
Copy link
Contributor Author

@Mytherin Thank you! Will confirm tomorrow once back to the machine.

@barracuda156
Copy link
Contributor Author

@Mytherin I cannot run tests at the moment, but yes, this works to fix the build, so all good for now.

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Mar 15, 2024
Merge pull request duckdb/duckdb#10758 from barracuda156/fix_macos
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.

2 participants