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.

Sign up for GitHub

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

Jump to bottom

Plugin dependencies in workspace cause main crate (and its deps) to be needlessly rebuilt #6143

Closed
crumblingstatue opened this issue Oct 6, 2018 · 1 comment · Fixed by #6170
Closed

Plugin dependencies in workspace cause main crate (and its deps) to be needlessly rebuilt #6143

crumblingstatue opened this issue Oct 6, 2018 · 1 comment · Fixed by #6170

Comments

@crumblingstatue
Copy link

crumblingstatue commented Oct 6, 2018 •
edited
Loading

I have a project that has a main application, and several dynamically loadable plugins for it.

The dependencies require plugin=true in the [lib] section of their Cargo.toml in order to behave correctly (they need to link libstd dynamically among maybe other things).

A reduced test case can be found at https://github.com/crumblingstatue/cargo-plugin-repro .

If you build it with cargo build --all to build the main application and all the plugins, it will generate the executable in target/debug, as expected.

Now if you try to run it with cargo run, it will rebuild the main application, and all of its dependencies, despite it already having been built, and placed in target/debug.

This is not a big deal for the reduced test case, but it's a huge deal for my main application, which has many dependencies that take a very long time to build.

Removing plugin=true makes the issue disappear.
@ehuss
Copy link
Contributor

ehuss commented Oct 6, 2018

I think I see what is happening (the used_in_plugin map is getting confused because things are used both for plugins and normal targets).

@crumblingstatue The plugin setting is intended for compiler plugins only. Writing application plugins is a bit tricky because the rust ABI is not stable. If you need to dynamically link plugins, I would recommend using cdylib and extern "C". Another approach is to use some sort of IPC between processes.

@crumblingstatue crumblingstatue mentioned this issue Oct 7, 2018
Closed
@ehuss ehuss mentioned this issue Oct 12, 2018
Merged
bors added a commit that referenced this issue Oct 15, 2018
@bors
Auto merge of #6170 - ehuss:panic-in-unit, r=alexcrichton
571a542 
Track `panic` in Unit early.

This is an alternate solution for #5445. It ensures that `panic` is cleared in the Profile for "for_host" targets (proc-macro/plugin/build-scripts) and dependencies. This helps avoid unnecessary recompiles in some situations (though does add extra units in some situations, see below).

Some examples where extra rebuilds are now avoided:
* A workspace with a dependency shared with normal and build-deps.  `build --all` should build everything, and then `build -p foo` was causing a recompile because the shared dep was no longer in the `used_in_plugin` set. Now it should not recompile.
* `panic=abort`, with a shared dependency in build and dev, `build` would cause that shared dependency to be built twice (exactly the same without panic set). Now it should only build once.

Some examples where `panic` is now set correctly:
* `panic=abort`, with a binary with a shared dependency in build and normal, `test` would cause that shared dependency to be built twice (exactly the same without panic set).  Now it is still built twice, but the one for the normal (bin) dependency will correctly have `panic` set.

Some examples where new units are now generated:
* `panic=abort`, with shared dependency between normal and proc-macro or build. Previously the shared dependency was built once with `panic=unwind`. Now it is built separately (once with `panic`, once without). I feel like that this is more correct behavior — that now the normal dependency avoids adding landing pads.

   For `panic=abort` cross-compiling, this makes no difference in compile time since it was already built twice.

Additional notes:
- I left build-scripts with the old behavior where `panic` is cleared for it and all its dependencies. There could be arguments made to change that (#5436), but it doesn't seem important to me.
- Renamed and refactored `ProfileFor` to `UnitFor`. I expect this API to continue to evolve in the future.

Closes #6143, closes #6154.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Track panic in Unit early. ehuss/cargo
2 participants
@ehuss @crumblingstatue