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

Simplify lint crates build to a single cargo build invocation #250

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Sep 19, 2023

cargo build can be specified to build the dependent crates only. It means we can use the build workspace to both fetch and build the lint crate dlls with a single cargo build command.

So instead of

cargo fetch
metadata=$(cargo metadata)
cargo build --manifest-path /path/to/lint-crate/a/Cargo.toml -Z unstable-options --out-dir path
cargo build --manifest-path /path/to/lint-crate/b/Cargo.toml -Z unstable-options --out-dir path
# ...

We have just

artifacts=$(cargo build --message-format json -p a -p b ...)
metadata=$(cargo metadata)

And then we pick the DLLs from the generated artifacts metadata.

This also removes the usage of the unstable --out-dir parameter for cargo build.

Draft

I need to fix the integration with --message-format json to discover the compiled artifacts. The problem space here is to match the package ID of the lint crates with the package ID of the compiler artifacts reports

`cargo build` can be specified to build the dependent crates only. It means we can use the build workspace to both fetch and build the lint crate dlls with a single `cargo build` command.

So instead of
```bash
cargo fetch
cargo metadata
cargo build --manifest-path /path/to/lint-crate/a/Cargo.toml
cargo build --manifest-path /path/to/lint-crate/b/Cargo.toml
```

We have just
```bash
cargo build -p a -p b
```

This also removes the usage of the unstable `--out-dir` parameter for `cargo build`.
@Veetaha Veetaha force-pushed the feat/simplify-lint-crates-build branch from e03bc41 to 072e308 Compare September 19, 2023 23:45
@Veetaha Veetaha marked this pull request as draft September 19, 2023 23:55
@xFrednet xFrednet added C-enhancement Category: New feature or request A-marker-cargo Area: All things connected to `cargo_marker` S-waiting-on-author Status: This is awaiting some action from the author. labels Sep 21, 2023
@xFrednet xFrednet self-assigned this Sep 21, 2023
@xFrednet
Copy link
Member

I need to fix the integration with --message-format json to discover the compiled artifacts. The problem space here is to match the package ID of the lint crates with the package ID of the compiler artifacts reports

It's a shame, that Cargo's metadata doesn't include the build artifacts and their names.

Here is another idea, which might be worth checking out:

Cargo has a --config parameter which can be used to override any field for the execution of a command. We could set the lib.name field of lint crates to get the file names we want. I've done some testing, just by editing marker_lints/Cargo.toml. The following worked for me:

Set lib.name field in marker_lints/Cargo.toml:

  [lib]
+ name = "cool"
  crate-type = ["cdylib"]

Run cargo build -p marker_lints. This creates the following .so artifacts:

  • target/debug/libcool.so
  • target/debug/deps/libcool.so

Parsing --message-format json might be the better approach, but I wanted to mention this option as well :)

@Veetaha
Copy link
Contributor Author

Veetaha commented Sep 22, 2023

I think using --message-format json is better, because this way we can build everything with a single cargo build, which should in theory be faster. I suppose your suggestion for overriding the --config implies multiple cargo builds, right?

But.. I suppose we will actually need multi-cargo build behavior if we want to implement --locked where we use the lock file from the lint crates themselves (#251 (comment)).

@xFrednet
Copy link
Member

I suppose your suggestion for overriding the --config implies multiple cargo builds, right?

Yes, I came up with the idea in that context, but the flag might be usable for dependencies as well. I assume that it can modify everything cargo-metadata provides. But this is just a guess. It was also a general FYI, that this option exists. :)

@Veetaha
Copy link
Contributor Author

Veetaha commented Oct 1, 2023

There is a problem with the --message-format approach. Error messages are emitted in JSON as well and cargo marker would need to forward them.
This complicates things even more. Need to think about this 🤔

@xFrednet
Copy link
Member

xFrednet commented Oct 2, 2023

Yeah, the ability to request this information is sadly quite limited 😕 I might have two ideas:

  1. Check if cargo maybe supports outputting two formats at once.
  2. The very hacky one: First run cargo build and forward all warnings etc to the user. If that succeeds, run it again with --message-format to get the artifacts. This might work with incremental compilation + caching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-marker-cargo Area: All things connected to `cargo_marker` C-enhancement Category: New feature or request S-waiting-on-author Status: This is awaiting some action from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants