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

Package-dependent compiler flags #10

Open
sgrove opened this issue Nov 28, 2015 · 4 comments
Open

Package-dependent compiler flags #10

sgrove opened this issue Nov 28, 2015 · 4 comments

Comments

@sgrove
Copy link
Contributor

sgrove commented Nov 28, 2015

Adding {"dependency": "core"} to findlibPackages will cause failure in a build, because the package demands either the -threads or -vmthread flag. I added a bit of code to be able to group these kinds of flags with their dependencies:

    , "findlibPackages": [{"dependency": "lambdasoup"}
                          , {"dependency": "core"
                          ,  "compileFlags": [" -vmthread "]}
                          , {"dependency": "cohttp"}
                          , {"dependency": "cohttp.lwt"}
                          , {"dependency": "lwt"}]

I'm not 100% sure it's a great idea. I like the explicit nature of this, and if different packages have conflicting packages, it's easier to point out as an exception. But compiler flags feel like a higher-level concept than packages, so I'm not sure about it.

@jordwalke
Copy link
Owner

So you're saying this is more organized than being required to put those flags in your package.json compilerFlags or linkFlags? Seems okay.
(Btw wouldn't you want to also allow linkFlags?)

It would be better if those dependencies themselves declared that any dependents must be compiled with particular flags or that the final executable must be linked with particular flags. Then all of that can be automatically added without you even having to know about it as the user of those dependencies. That's the kind of stuff we can do when everything uses CommonML.

@sgrove
Copy link
Contributor Author

sgrove commented Nov 28, 2015

No, as it stands right now, compileFlags won't work for this (happy to be corrected if I'm wrong). My code/proposed change was trying to work around this:

package.json
{
  "name": "MyProject"
  , "dependencies": {"CommonML": "git://github.com/jordwalke/CommonML.git"},
  "CommonML": {
    "exports": ["MyPublicModule"]
    , "findlibPackages": [{"dependency": "lambdasoup"}
                          , {"dependency": "core"}
                          , {"dependency": "cohttp"}
                          , {"dependency": "cohttp.lwt"}
                          , {"dependency": "lwt"}
                          , {"dependency": "utop"}]
    , "compileFlags": ["-vmthread"]
    , "linkFlags": []
  }
}

(hacked build output)

$ node node_modules/CommonML/build.js

Building Root Package /Users/s/src/MyProject [byte]

Scanning files from /Users/s/src/MyProject

Building dependency packages for MyProject

ocamldep  ->  false
FLAGS -package lambdasoup -package core -package cohttp -package cohttp.lwt -package lwt -package utop
Final findLib:  ocamlfind ocamldep  -package lambdasoup -package core -package cohttp -package cohttp.lwt -package lwt -package utop -only-show
> Computing dependencies for MyProject


ocamldep.opt -sort -one-line  -impl /Users/s/src/MyProject/src/MyPublicModule.ml    -impl /Users/s/src/MyProject/src/mytop_main.ml
[STDOUT]:
 /Users/s/src/MyProject/src/MyPublicModule.ml /Users/s/src/MyProject/src/mytop_main.ml


ocamlc  ->  true
FLAGS -package lambdasoup -package core -package cohttp -package cohttp.lwt -package lwt -package utop
Final findLib:  ocamlfind ocamlc  -package lambdasoup -package core -package cohttp -package cohttp.lwt -package lwt -package utop -only-show
ocamlfind: Error from package `threads': Missing -thread or -vmthread switch
Uncaught exception.

Stack Trace:
------------
 Error: Command failed: ocamlfind ocamlc  -package lambdasoup -package core -package cohttp -package cohttp.lwt -package lwt -package utop -only-show
ocamlfind: Error from package `threads': Missing -thread or -vmthread switch

    at checkExecSyncError (child_process.js:1276:13)
    at Object.execSync (child_process.js:1316:13)
    at getFindlibCommand (/Users/s/code/CommonML/build.js:1098:24)
    at buildFromDependencies (/Users/s/code/CommonML/build.js:1802:9)
    at onDepsDiscoveryDone (/Users/s/code/CommonML/build.js:1910:9)
    at onOneOcamldepDone (/Users/s/code/CommonML/build.js:1534:5)
    at executeScripts (/Users/s/code/CommonML/build.js:1451:5)
    at /Users/s/code/CommonML/build.js:1481:9
    at ChildProcess.exithandler (child_process.js:676:7)
    at emitTwo (events.js:87:13)

@jordwalke
Copy link
Owner

So what does the correct compile command look like?

@jordwalke
Copy link
Owner

Yeah I can see why this would be good to have.

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

No branches or pull requests

2 participants