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

Respect user's environment FLAGS #22

Closed
wants to merge 1 commit into from
Closed

Conversation

matoro
Copy link
Contributor

@matoro matoro commented Jul 15, 2022

Hi, currently ffi-compiler does not respect the CFLAGS set by a user or build system and instead builds its extensions with its own hardcoded set of flags. This should have it properly respect the flags that a user requests to build with.

@thesamesam
Copy link

@larskanis @davispuh Would you mind taking a look? We'd like to pull this in downstream in Gentoo but hoping for a review on the PR first. Thanks in advance

@davispuh
Copy link
Collaborator

Looks fine to me but note that it won't work correctly if there is any spaces since #split won't take into account quoting (ie. LDFLAGS=-L "/mnt/path with spaces").

Also not sure how we should handle this since while I can merge it I don't really consider myself a maintainer of this project :d but I guess reality is that there are none...

Anyway I guess I'll wait till end of April and if no one objects I'll merge it then.

@matoro
Copy link
Contributor Author

matoro commented May 3, 2023

Looks fine to me but note that it won't work correctly if there is any spaces since #split won't take into account quoting (ie. LDFLAGS=-L "/mnt/path with spaces").

Also not sure how we should handle this since while I can merge it I don't really consider myself a maintainer of this project :d but I guess reality is that there are none...

Anyway I guess I'll wait till end of April and if no one objects I'll merge it then.

@davispuh Should I change this to .shellsplit instead of .split?

@davispuh
Copy link
Collaborator

davispuh commented May 3, 2023

That's actually good idea, while it doesn't exactly give the semantics we would want it definitely would work better and it would correctly handle spaces/quoting.

Unfortunately it turns out that it wouldn't have mattered since it already was broken regarding space/shell handling, I just fixed that in 5e173ae
and I also updated your commit and merged it in, see 7caa353

But overall I would say that whole compile_task.rb needs rewriting, it's pretty bad idea to do it like this with concatenating strings and passing them directly to shell but I guess without any maintainers it's good enough 😅

@davispuh davispuh closed this May 3, 2023
@matoro
Copy link
Contributor Author

matoro commented May 3, 2023

Cool, @thesamesam would you mind updating the patch we are applying since it does not seem like this will get a new release anytime soon?

@davispuh
Copy link
Collaborator

davispuh commented May 3, 2023

I just updated and tagged version, you can pull in from https://github.com/ffi/ffi-compiler/releases/tag/1.2.0

I don't have permissions to publish RubyGem, but with Bundler you also can do

gem 'ffi-compiler', :git => 'https://github.com/ffi/ffi-compiler.git', :tag => '1.2.0'

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