search

ffi / ffi-compiler

Apache License 2.0
32 stars 10 forks source link

Respect user's environment FLAGS #22

Closed matoro closed 1 year ago

matoro commented 2 years ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 5e173ae1de068bea5890c4c2438c5b1e1cdad702 and I also updated your commit and merged it in, see 7caa3535ad0c4adfe8b4aef3a2179a39adaa78af

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 :sweat_smile:

matoro commented 1 year ago

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 commented 1 year ago

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'