Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd Cargo post-build scripts #1777
Conversation
This comment has been minimized.
This comment has been minimized.
|
I kinda dislike the idea of cargo becoming your average generic build system. |
japaric
reviewed
Oct 26, 2016
| ## Use cases | ||
|
|
||
| On most platforms, once linking is done there is no longer any work to do. This | ||
| differs on some systems, such as AVR (which requires linked ELF binaries to be |
This comment has been minimized.
This comment has been minimized.
japaric
Oct 26, 2016
Member
I think this would be better handled by a Cargo subcommand. The rationale is that even if Cargo would produce a raw binary blob as its final artifact you'll still need to call a second tool to flash that into a device. So, IMO, we could leave things as they are (final artifact = ELF) and have a Cargo subcommand, say cargo flash, that does the ELF -> binary file part and then flashes that binary into the device.
This comment has been minimized.
This comment has been minimized.
dylanmckay
Oct 27, 2016
Author
I think this would be better handled by a Cargo subcommand
That's a good idea, I like it
japaric
reviewed
Oct 26, 2016
| differs on some systems, such as AVR (which requires linked ELF binaries to be | ||
| converted into raw binary blobs). | ||
|
|
||
| Currently this would require something like a Makefile which internally |
This comment has been minimized.
This comment has been minimized.
japaric
Oct 26, 2016
Member
Could you elaborate on this? It seems to if the Makefile uses Cargo then there are crates within the repository / project directory and those crates can be published to crates.io.
japaric
reviewed
Oct 26, 2016
| Currently this would require something like a Makefile which internally | ||
| calls `cargo`, which prohibits the crate being published to `crates.io`. | ||
|
|
||
| In another case, we could use post-build scripts to generate mountable disk |
This comment has been minimized.
This comment has been minimized.
japaric
Oct 26, 2016
Member
For development, a Cargo subcommand like the one I mentioned above could fit the bill. cargo qemu could string executables together into a .iso file and then spawn a QEMU instance that boots that disk.
For deployments, I think that's very likely that one will need to use shell scripts or Makefiles to produce the final tarball, checksum it, sign it, upload it, etc. so I don't see much gain from having Cargo help with an extra step of doing the executable -> .iso file part.
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
burdges
commented
Oct 26, 2016
|
Another reason to skip this : Any build script is a potential security threat, especially if they are being automatically downloaded and run. There are people who would build unfamiliar code in one QubesOS VM and run in another, at least when working in a language like Go that does not support automatic build scripts. There is a case for global configuration options that either disable all build scripts, or prompts the user, give them a chance to review the script, and record the results in Cargo.lock or someplace more secure. |
This comment has been minimized.
This comment has been minimized.
I didn't know that, nor do I agree with it. It would be really cool to see cargo as a fully-fledged and flexible build system, but I can see the arguments against it.
This is already an issue with the current build script setup, this change wouldn't be any less secure. |
This comment has been minimized.
This comment has been minimized.
camlorn
commented
Oct 27, 2016
|
Some thoughts: Firstly, this is very unspecified. How does the post-build script get information out of cargo? What information is available? Secondly, I would prefer something a bit different: have Cargo learn how to write json to somewhere (probably stdout). Put the location of all build artifacts that are important in this json. Then, provide a way for a build script to add custom keys to this json, i.e. paths to art assets or extra artifacts for Windows dlls or whatever. this would then allow the community to produce cargo install commands or other tools that consume this information, including things to do what this RFC is attempting to accomplish. Other examples include Windows installers, maybe OS X fat binaries (where the install command builds the package multiple times), packers, etc. This would also make it easier to integrate cargo into things like CMake, allowing me and others to use Rust as a dependency in C/C++ projects on platforms like Windows where there is no standard install location. Once the community has built these tools and settled on a schema, we can bring the schema in via RFCs, possibly also pulling in tools. I would also like to see cargo grow an API for this kind of thing so that we can eventually deprecate the environment variables approach. I think this RFC isn't necessarily bad, but it doesn't go very far at all in terms of doing things beyond the specific use cases in the RFC, nor does it allow one to swap out post-build scripts if one wishes to use a crate for multiple things that need one. Finally, in the specific case of embedded targets that need custom steps, Rust should probably just grow support for them in the target triple (falling under the better embedded situation in the roadmap RFC). |
This comment has been minimized.
This comment has been minimized.
burdges
commented
Oct 28, 2016
|
Anything that encourages more automated build scripts potentially increases the security threat @dylanmckay if only because an option that disables automated build scripts now breaks more packages. A post-build script can hide the nefarious bits throughout the main project, while claiming to do tests or something. As an example going the other way, we might add a |
This comment has been minimized.
This comment has been minimized.
camlorn
commented
Oct 28, 2016
|
@burdges To elaborate on my alternative (which I've been debating doing an rfc for--maybe I should): Your tool does, say,
And potentially add something to allow build scripts to advertise additional key-value pairs. This has no security problems that I can see and accomplishes what is needed here as a |
This comment has been minimized.
This comment has been minimized.
jmesmon
commented
Oct 28, 2016
•
Edit: I'd also like to note that allowing post-build processing doesn't really make |
This comment has been minimized.
This comment has been minimized.
camlorn
commented
Oct 28, 2016
|
@jmesmon The other advantage of allowing a tool to wrap Cargo is that you have to explicitly run the other tool. Allowing post-build scripts means that every single direct or indirect dependency I use is a point of entry for a malicious attacker. Having upwards of 100 dependencies can happen. This means 100 points where someone might put an insecure or virus build script. Or Cargo learns to spit out information, and the only things that run are things that I explicitly installed and ran myself--i.e. for the cases of this RFC, one tool. And finally, my alternative leads to reusable tools that just work. A post-build script can't be installed into your package by depending on another package. There is no way for an author of a package that needs to be called from a (post)-build script to maintain the script themselves without your involvement. If libraries down the chain of dependencies also use them, you may be at the mercy of your dependencies. But a tool can just be upgraded and, presuming the author of the tool didn't break it, it'll still do what it did before. Without any source changes to your package or, worse, to packages that you do not directly control. At worst, this RFC leads to everyone maintaining private forks of stuff so they can make the (post)-build scripts cooperate. |
This comment has been minimized.
This comment has been minimized.
camlorn
commented
Oct 29, 2016
|
Just thought of this. I'm linking the HN thread because the article isn't loading right now. The basic idea is, for example, registering Whether anyone is using such holes? Who knows, but it does demonstrate that this is a thing that can be done. I could see it being reasonable to allow full disabling of build scripts in the future, for environments where you really care, and anything that adds more places for code to magically run...I'm not exactly against it because it is useful, but it isn't exactly all sunshine and rainbows. As an example of a practical attack, claim you're running tests, execute git commands that completely clobber my local history and destroy my source code, then run Of course this can be done with current build scripts, but adding yet another palce where it can be done and for which disabling it can break some packages...I don't know. Wiser minds will need to weigh in on the security issue given that these holes are already present. Maybe making Cargo confirm that yes, you really do want to run current build scripts and/or offering an option to require explicit whitelisting is a good idea. |
This comment has been minimized.
This comment has been minimized.
|
Related issue and discussion: rust-lang/cargo#545 |
JinShil
reviewed
Nov 1, 2016
|
|
||
| ## Use cases | ||
|
|
||
| On most platforms, once linking is done there is no longer any work to do. This |
This comment has been minimized.
This comment has been minimized.
JinShil
Nov 1, 2016
Contributor
Another common use case for embedded systems is to display the binary size information (e.g. arm-none-eabi-size the_binary) after each build.
This comment has been minimized.
This comment has been minimized.
|
another alternative: Cargo execute build.rs with extra arg, e.g. |
nikomatsakis
added
the
T-dev-tools
label
Nov 2, 2016
This comment has been minimized.
This comment has been minimized.
|
This RFC is quite old by now, and we haven't heard from anybody involved actively in Cargo leadership. @alexcrichton @carols10cents @wycats, do you have thoughts? |
This comment has been minimized.
This comment has been minimized.
|
I would echo the concerns of @nagisa and @steveklabnik of how it's not intended for Cargo to become a full-fledged I sympathize with @jmesmon's concerns in that |
This comment has been minimized.
This comment has been minimized.
If this isn't within the scope of Cargo, where, in the Rust ecosystem, does such a feature belong? Is there really room in the Rust ecosystem for a build manager in addition to Cargo? And, if so, where is the line drawn between the two? |
This comment has been minimized.
This comment has been minimized.
|
rust-lang/cargo#3815 is the proper solution, @JinShil. Insofar that this RFC would be used for end projects, not redistributal libraries, use that feature to extract a build plan and then add to it. |
This comment has been minimized.
This comment has been minimized.
|
Since it's possible for this to be implemented with a |
This comment has been minimized.
This comment has been minimized.
|
@rfcbot fcp close |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
May 2, 2017
•
|
Team member @carols10cents has proposed to close this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
This comment has been minimized.
This comment has been minimized.
|
@JinShil for a generic "post build" script I think @Ericson2314's suggestion of extending build plans would be where that would get inserted. There would likely be something like a maintained tool to generate a build system from Cargo, and then specific build systems could have hooks for things like post-build scripts. For a less general solution like "just run this one script after it's all said and done for the end project" then @carols10cents's idea of just a custom cargo subcommand I think is the way to go. |
This comment has been minimized.
This comment has been minimized.
|
I think there are real use cases to solve here, though I'm not opposed to continuing to punt on it. I also have often felt a need for post-install scripts. One strong concern I have is that I would like to see more success with exporting cargo's build plan to other build systems, and understanding how that can work, before adding more dynamism to cargo's build plan. |
This comment has been minimized.
This comment has been minimized.
|
I would be in favor of something like this. I also don't understand why "Cargo is not a generic build system" argument is being brought up against post-build scripts, whereas existing build scripts were just fine... |
This comment has been minimized.
This comment has been minimized.
|
I think it's a very slippery slope adding more build-system-like features. One could also argue that Cargo is 75% of make today. It's checking timestamps, has parallel builds, caching artifacts, etc. However extending it further means that the design for Cargo will never stop, in my opinion. I agree that this feature is critical for libraries with postprocessing logic, but there's not a lot of examples of that so far. We, for example, have proposals for embedding resources in windows dlls via other mechanisms than a post build script (which are much more ergonomic than a post build script as well). |
This comment has been minimized.
This comment has been minimized.
|
@vadimcn I recall in the thread on generating build plans it was discussed that the existing build scripts already impede generation of a single static build plan. |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton: yes, we may have covered current use cases, but I am sure something new will come up. I like having an escape hatch... @Ericson2314: Whatever there solution will be for build scripts, we could apply it to post build too, no? |
This comment has been minimized.
This comment has been minimized.
|
@vadimcn I'm saying there may not be a full solution for build scripts. |
This comment has been minimized.
This comment has been minimized.
michaelwoerister
commented
May 8, 2017
|
I'm on the fence about this one. Post-build scripts seem like a useful feature that might not be realized without being integrated directly into Cargo. But I also understand concerns about growing Cargo's feature set in this respect, especially when integration into other build systems isn't fleshed out yet. My suspicion is that per-crate post build scripts would work just fine when integrated into larger build systems and would not add more complications on top of regular build scripts, since they should only do "self-contained" things pertaining to the one crate being currently built. But I would not object not merge this RFC until the build system story is more fleshed out. |
This comment has been minimized.
This comment has been minimized.
|
I think the whole RFC needs more fleshing out: It doesn't specify how Cargo should pass information about the executables it generated as part of its normal build process to the post-build script. Environment variables? Or is the post-build script supposed to hard-code the name of Cargo's output artifacts in it? It doesn't specify what mechanism would be used to change the post-build script behavior when the build target changes. Surely the post-build script should behave differently if one's is building a library ( It doesn't specify if or how dependencies' post-build scripts would work. If a dependency's post-build script generates some file from an .rlib, how is this new file supposed to be used in the rest of the Cargo build? Or perhaps post-build scripts should only be allowed to modify .rlibs, .dylibs, etc. in place. I personally don't find the motivation examples compelling. In both cases even if the post-build script processes Cargo's output you still can't directly use the post-build output (in the AVR example, you have to call some flashing tool and in the OS example, you have to run the .iso in an emulator), whereas with normal Cargo builds you get an executable that you can directly run. If you have to call an external tool to use Cargo's output whether post-build scripts exist or not then I don't see a strong motivation to put this post-processing in Cargo instead of pushing it into the external tool. I personally would rather not complicate Cargo build process further if that's the case. I would suggest implementing (out of tree) a general @rfcbot reviewed |
This comment has been minimized.
This comment has been minimized.
|
@rfcbot reviewed (I typo-ed the bot name in the previous comment and fixed it in an edit, but it seems the bot didn't pick up the command from the edit so trying again) |
This comment has been minimized.
This comment has been minimized.
|
@rfcbot reviewed |
1 similar comment
This comment has been minimized.
This comment has been minimized.
michaelwoerister
commented
May 15, 2017
|
@rfcbot reviewed |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
May 15, 2017
|
|
rfcbot
added
the
final-comment-period
label
May 15, 2017
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton, I don't feel strongly enough about it to continue arguing in favor of this particular RFC. I dunno, maybe there should be an Also, as @japaric, posted above, the behavior of post-build scripts is currently under-specified, so at least that would need updating. |
This comment has been minimized.
This comment has been minimized.
rfcbot
commented
May 25, 2017
|
The final comment period is now complete. |
This comment has been minimized.
This comment has been minimized.
|
Ok! Looks like not a lot of new discussion came up during FCP, so I'm going to close. Thanks again though for the RFC @dylanmckay! |
dylanmckay commentedOct 26, 2016
•
edited
This adds support to have a separate build script run after compilation completes.
Rendered