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: add -z noexecstack in Makevars #1212

Merged
merged 7 commits into from
Aug 30, 2024
Merged

fix: add -z noexecstack in Makevars #1212

merged 7 commits into from
Aug 30, 2024

Conversation

etiennebacher
Copy link
Collaborator

Error seen in ubuntu CI workflow of tidypolars

@etiennebacher etiennebacher changed the title fix: add additional flag in Makevars fix: add -z noexecstack in Makevars Aug 30, 2024
src/Makevars.in Outdated
Copy link

Choose a reason for hiding this comment

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

Hello! The flag you found goes to the linker, not cargo itself. The location should be somewhere that gets passed to R's build step of the package, in which the linker is invoked (oddly enough, the linker interface is gcc too lol). I think you can start by putting this in PKG_LIBS line and see if it helps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, let's see how it goes

Copy link
Collaborator Author

@etiennebacher etiennebacher Aug 30, 2024

Choose a reason for hiding this comment

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

@CGMossa it seems that this flag only exists on Linux (just my guess since ubuntu passes but macos workflows fail with ld: unknown options: -z ). Any idea how to fix it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, I think this option should go in the configure file, no makevars.in

@etiennebacher etiennebacher merged commit 5ccab18 into main Aug 30, 2024
@etiennebacher etiennebacher deleted the makevars branch August 30, 2024 17:31
@CGMossa
Copy link

CGMossa commented Aug 30, 2024

This is so good to know.... I'm wondering if this needs to be upstreamed to extendr.

@etiennebacher
Copy link
Collaborator Author

etiennebacher commented Aug 30, 2024

I still don't know if this worked, I need to wait for 0.19.1 to be available on R-universe. Also, this is the first time I see this, I never had a glibc issue in CI before. But you know more than me what should and shouldn't be handled in extendr (might be part of a FAQ / "good to know" section in the docs).

@@ -118,5 +118,5 @@ Collate:
'zzz.R'
Config/rextendr/version: 0.3.1
VignetteBuilder: knitr
Config/polars/LibVersion: 0.42.0
Config/polars/LibVersion: 0.42.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this change necessary?
Isn't this flag unrelated to the Rust binary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it wasn't, I think I misinterpreted the failures in the first commit because I've been bitten before by compilation errors when I forgot to update the libversion

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.

3 participants