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

Allow the user to define arbitrary C variables passed to c_driver_program. #652

Closed
wants to merge 1 commit into from

Conversation

julie-is-late
Copy link

Since PC.jl now exposes c_driver_program which allows the users to specify their own C wrapper program, it would be nice to be able to pass in more variables which might be best defined at build time.

For my use case, I would like to bake in a build version at build time. And rather than storing it in julia or needing to compile it into a file, it is far easeier to just have something like:

  #ifdef VERSION
      printf("%s\n", VERSION);
  #else
  ...

Where VERSION would need to be passed to the compiler like: -DVERSION="1.3.0".

This change will allow for that by passing a c_defines dict and letting the user pass in arguments for the compiler to define as a macro.

Since PC.jl now exposes `c_driver_program` which allows the users to
specify their own C wrapper program, it would be nice to be able to pass
in more variables which might be best defined at build time.

For my use case, I would like to bake in a build version at build time.
And rather than storing it in julia or needing to compile it into a
file, it is far easeier to just have something like:

  #ifdef VERSION
              printf("%s\n", VERSION);
  #else

This change will allow for that by passing a `c_defines` dict and
letting the user pass in arguments for the compiler to define as a
macro.
@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #652 (8aca9e7) into master (22576ee) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #652      +/-   ##
==========================================
+ Coverage   95.25%   95.29%   +0.04%     
==========================================
  Files           2        2              
  Lines         464      468       +4     
==========================================
+ Hits          442      446       +4     
  Misses         22       22              
Impacted Files Coverage Δ
src/PackageCompiler.jl 95.76% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22576ee...8aca9e7. Read the comment docs.

@KristofferC
Copy link
Member

Can't you already do this by setting an appropriate JULIA_CC ENV var?

@julie-is-late
Copy link
Author

How would that work? setting it like ENV["JULIA_CC"] = "/usr/bin/gcc -DVERSION=1.0"?

it does seem like the way the variable is handled in run_compiler would allow for that, however at least to me that feels very unintuitive. I not only would not have known to try it, but also to me that feels more like an option that explicitly is "the location of the compiler binary" and not "the beginning of the compile command".

Further, if I'm on windows I really want to leave the logic alone for using gcc from the local mingw. Otherwise I'm hard-coding a magic directory string for where it will be or adding the dependency to my own project.

@KristofferC
Copy link
Member

Okay, what I don't want is to add a zillion of keyword arguments for all kind of compiler features. There should in my opinion be one at most one argument for advanced users where they can customize the compiler options.

Also, the docs do say:

An attempt to automatically find a compiler will be done but can also be given
explicitly by setting the environment variable JULIA_CC to a path to a
compiler (can also include extra arguments to the compiler, like -g).

@julie-is-late
Copy link
Author

julie-is-late commented Dec 15, 2021

very fair. and I must have just missed that in the docs, however I still feel like that makes those options inaccessible on windows.

As a long term solution, why not organize the compiler flags and such into some sort of organized object/struct, where all options like binary location, environment, flags, cpu target, etc are stored. The current functions as is could just keep complexity away from the user and simply construct it in place and then delegate off to a version which accepts the struct, but an advanced user could manage their own versions of it if they wanted and then call directly into the versions which use the struct?

Alternatively just formalize a new environment variable which is directly intended to pass arguments instead of being dual purpose?

@julie-is-late
Copy link
Author

how would you feel if instead this looked something like:

diff --git a/src/PackageCompiler.jl b/src/PackageCompiler.jl
index 4068520..4d0dbb1 100644
--- a/src/PackageCompiler.jl
+++ b/src/PackageCompiler.jl
@@ -118,6 +118,7 @@ const WARNED_CPP_COMPILER = Ref{Bool}(false)

 function run_compiler(cmd::Cmd; cplusplus::Bool=false)
     cc = get(ENV, "JULIA_CC", nothing)
+    cc_flags = get(ENV, "JULIA_CC_FLAGS", "")
     path = nothing
     @static if Sys.iswindows()
         path = joinpath(LazyArtifacts.artifact"mingw-w64", (Int==Int64 ? "mingw64" : "mingw32"), "bin", cplusplus ? "g++.exe" : "gcc.exe")
@@ -158,7 +159,7 @@ function run_compiler(cmd::Cmd; cplusplus::Bool=false)
     if path !== nothing
         compiler_cmd = addenv(compiler_cmd, "PATH" => string(ENV["PATH"], ";", dirname(path)))
     end
-    full_cmd = `$compiler_cmd $cmd`
+    full_cmd = `$compiler_cmd $cc_flags $cmd`
     @debug "running $full_cmd"
     run(full_cmd)
 end

@julie-is-late
Copy link
Author

Closed in favor of #665

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.

2 participants