Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 "artifact dependencies" on bin, cdylib, and staticlib crates #3028
Allow "artifact dependencies" on bin, cdylib, and staticlib crates #3028
Changes from 1 commit
b921f08
fac809f
d45fb22
0670af0
9bc481d
1056622
c6fde3c
b06d9cb
42ae02f
8faaab8
6d95b60
2e1aabf
32f1a50
983638e
c547a58
22c8a57
9aebb5a
c46b068
442dcb0
4e6f6ea
7f51fc4
58577cb
ff2b254
82baedf
0f3933b
6c84436
eaf80fb
be85559
93c3094
c9470ad
a42fead
98e1476
2ddc224
278de5a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this RFC is necessary in this case for binaries defined in the local package. All integration test targets implicitly depend on all binary targets in a package today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Twey might have been talking about using binaries in unit tests. I'll let them describe their use case, I don't know anything other than the linked comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this perhaps be expanded a bit? I'm not really sure what this is referring to, because
cargo build
builds all the binaries today in a package. Does rustc-perf have different packages with binaries that depend on each other?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, the binaries depend on each other, so you can't use
cargo run
because that requires a specific binary.cargo build
does work, but means you have to run the binary astarget/release/collector
, so it's easy to forget to recompile after a change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried about cdylib/staticlib here in the sense that these often have canonical meanings which Cargo would otherwise not be interpreting. For example if you depend on a
lib
Cargo will pass all the right--extern
flags and make sure that it makes its way to the final binary. With acdylib
and/or staticlib you may want that as well.For example you might want to consume a crate's C API and want to have that automatically linked into the final format. By only supporting
cdylib
andstaticlib
here and nothing else about these crate types I'd fear that we'd get into a situation where depending on one of these crate types involves a lot of boilerplate that Cargo could otherwise handle if we took some time to game out what it meant.Is it possible to game out here what the main use cases are here and see if we can improve the Cargo experience?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton This isn't intended for the use case where you just want to link your binary (or library) to the produced library. For that use case, I absolutely agree that cargo should provide more integration. This is intended for the use case where the library serves some other function, such as a runtime library for an interpreter (linked into interpreted programs rather than just the interpreter), or a VDSO for a kernel (linked into userspace programs run on that kernel), or a preloaded library for use with
LD_PRELOAD
, or a remote debugging stub injected into another program or device. In those cases, you really do just want the compiled artifact.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the problem lies exactly within
type="lib"
providing some kind of integration (adding--extern
torustc
invocations) andtype="cdylib"
ortype="staticlib"
not doing so. Or vice versa. This difference is not easily inferred by just reading a toml file that contains both or one of the settings.And yet the options change enough about the build process that anybody who attempts to understand the dependency structure needs to be very aware of the difference in behaviour here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nagisa I don't think it's reasonably possible to have "default" handling for
cdylib
, because linking a shared library doesn't guarantee it'll be available at runtime.That said, if we believe there's some value in having non-artifact dependencies on cdylib or staticlib, with automatic handling, we could flag artifact dependencies in some way. For instance,
type="artifact:cdylib"
. We could then reservetype="cdylib"
for something else. I don't think that's necessary, but it's a reasonable possibility, and this feature would still be suitable for the intended purposes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option that comes to mind is to just remove
type="lib"
for now – making the default integrated beaviour only happen when thetype
is absent; or perhaps make it behave much like other types, in that it would only provide therlib
as an artifact, without any integrated behaviour.In that case people who want both the "integrated" behaviour as well as an artifact, they could specify the dependency multiple times, once with a
type
and once without.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One worry I would have about supplying a
target
feature is that this sort of attribute is often better placed on the package itself. For example if you're depending on a wasm blob the package that compiles to wasm is unlikely to ever not want to get compiled to wasm, but this means that you'd have to specify the target every single time you depend on it.I think that this sort of attribute on a dependency would work but it isn't clearly the best solution available to us I think. I think it would be worthwhile to explore the use cases of this and see if they'd be better suited with a dependency-level or package-level target declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting something like
[package] default-target = "wasm32-wasi"
? I'd be interested in that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree: for power-cpu-sim (linked above), it is specifically designed to be able to compile to a command-line program for linux/etc. and the exact same crate is also designed to be compiled for the web using target wasm32-wasi and there is an example web server which runs on linux and requires the produced wasm file to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I'm not saying that a dependency-level directive isn't useful. Nor does one reason why it is useful mean it's more useful than a package-level directive. My point is that the package-level directive is probably useful in more cases.
@jyn514 yes that's what I'm thinking (subject to bikeshedding of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexcrichton A package-level directive for the default also makes sense, but that would be a separate proposal. If a package truly supports only one target, a package-level directive would suffice. The
target
attribute on a dependency is intended for cases where the dependency supports multiple targets.For example, consider a crate that builds virtual machine firmware, and supports multiple architectures. If you're building a virtual machine for a specific architecture, you need firmware for that architecture.
Or, consider a kernel that supports both 64-bit and 32-bit userspace applications, and needs to build different versions of a VDSO for each case.
I also expect
target = "target"
on build dependencies to be quite common: "no, I don't want to run this as part of the build process, I want this to be runnable on the target system instead".I would also expect to see cases like this: