deploy_scripts#765
Conversation
…t the build directory can be packaged as a standalone toolchain.
|
Actually, given that Travis is saying something else red about #771, updated this PR to carry the above change. |
|
Wouldn't it be more conventional to just use CMake install targets instead? IIRC In some cases CMake actually does useful things when it installs (e.g. rewriting rpaths), so we probably should prefer that to just copying the build directory when we make distribution packages. |
|
The CMake install step would probably just complicate here, since that would require doing an install before being able to use the build products. I don't think it's worth to start complicating the build steps more. |
|
Hm. I do still think we want to have a working install step, but I agree that it's nice to also be able to use the build products in place for testing. So maybe we can just do both. |
|
Also (and this is beyond the scope of this particular PR) but |
|
They are referenced e.g. in https://github.com/kripken/emscripten/pull/4623/files#diff-ae40e645f84e893ce5d037b5efc53be6R1973. I did not want to change the directory structure more than necessary (just look at both |
|
Yeah, for this PR it's fine. I'm just saying I think we should move them in the future, and update the references. |
| not_executable_suffix = ['.txt', '.js'] | ||
| executables = sorted(filter(lambda x: not any(x.endswith(s) for s in | ||
| not_executable_suffix), | ||
| not_executable_suffix) and os.path.isfile(x), |
There was a problem hiding this comment.
does this check it isn't a directory?
did something change in this PR that adds a new directory?
There was a problem hiding this comment.
Yes, the FILE(COPY) command below adds new directories under bin/, for example bin/js.
| INSTALL(TARGETS wasm-dis DESTINATION bin) | ||
|
|
||
| # Deploy scripts that are utilized at runtime to bin/ directory, so that the build directory can be packaged as a standalone toolchain. | ||
| FILE(COPY "${CMAKE_CURRENT_SOURCE_DIR}/scripts" DESTINATION "${PROJECT_BINARY_DIR}/bin/") |
There was a problem hiding this comment.
at what time would these operations happen?
There was a problem hiding this comment.
These occur right during the CMake configure step.
There was a problem hiding this comment.
Oh right, during configure? That's not going to work if the scripts are updated, will it? Is there something that forces a reconfigure if they change, is that automatic?
There was a problem hiding this comment.
Unfortunately there is no simple spot to do it for dynamic copying. Being at configure time means that after making a change, one can reissue cmake to redeploy, so that's something that will always occur on CI etc.
The full CMake'y way with dependency analysis would be to create build targets out of them and have the build action be a copy command with add_custom_command(OUTPUT, ...), but that is a bit messy because it needs a target to build the files in, so either create a virtual target that only has the copyable files, or piggyback on another target project to build. If this is a concern, I can take a peek, but generally prefer to keep it simple.
There was a problem hiding this comment.
At first I thought it's not urgent to do the better fix, as normally we don't use code from the bin/ dir. But actually this + the emscripten PR makes us use stuff from bin/, like the js glue code file? That means that as I debug it, I'd need to remember to manually do cmake all the time - which sounds error prone and burdensome.
And actually, even having to run make to get the js glue code file to be copied to bin/ seems like a worse developer experience than we have now.
Overall it seems the old way, of using the js glue from the src dir, etc. and not using more stuff in bin/ is better - except for when creating a distribution, I guess. So doing something special for a distribution might be an alternative?
There was a problem hiding this comment.
I still think having install targets is the 'right' way to do this.
When creating a distribution, we don't want to depend on a source dir, and I don't think we want this stuff in the bin dir; we want it in some other dir: probably lib, share or some subdir of one of those. For local testing, I don't care too much; we can add something to check.py to handle things to avoid having to run the install step if we want, I'm happy to always do out-of-tree builds, or whatever.
There was a problem hiding this comment.
Alright, let me retract this pull request and figure something better out.
Deploy scripts that are utilized at runtime to bin/ directory, so that the build directory can be packaged as a standalone toolchain.
At present
-s BINARYEN_ROOTneeds to point to the root directory of a Binaryen git repository. When bundling Binaryen for distribution, we'd like to minimize the number of extra files, so having the outputbin/directory be a root of all distributable build artifacts would be a clean way to do it. These copy steps deploy all files needed at runtime to thebin/directory.