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

core/compiler: report executable paths for all binary crates #13605

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dvdhrm
Copy link
Contributor

@dvdhrm dvdhrm commented Mar 19, 2024

Ensure the compiler-artifact message reports executable: "..." for all builds where the crate-type is set to bin.

Currently, only crates with a target-type of bin will report the executable path. This is unfortunate, since several other builds produce executables that are otherwise hard to distinguish from other artifacts due to the lack of file extension.

In particular, the following builds now properly report their executables:

  • Library targets built via cargo rustc --crate-type bin
  • Build scripts

Before they would report executable: null and thus make it very hard for callers to decide whether the produced file is an executable.

Ensure the `compiler-artifact` message reports `executable: "..."` for
all builds where the crate-type is set to `bin`.

Currently, only crates with a target-type of `bin` will report the
executable path. This is unfortunate, since several other builds produce
executables that are otherwise hard to distinguish from other artifacts
due to the lack of file extension.

In particular, the following builds now properly report their
executables:

 - Library targets built via `cargo rustc --crate-type bin`
 - Build scripts

Before they would report `executable: null` and thus make it very hard
for callers to decide whether the produced file is an executable.
@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2024
@epage
Copy link
Contributor

epage commented Mar 19, 2024

Generally, I'd recommend creating an issue first for us to focus the discussion on the problem you are facing and what the potential solutions are.

Could you elaborate on why this is needed?

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Mar 20, 2024

Could you elaborate on why this is needed?

We build application bundles for a range of operating systems, including macOS Bundles, iOS Bundles, Windows msix, Android APK/Bundles, .... We post-process the artifacts generated by Cargo. We use cargo metadata to assemble the dependency tree that makes up a build, and then filter the artifacts reported by cargo build --message-format=json based on this tree. The artifacts are then post-processed (if needed) and assembled in the final bundle.

This setup keeps the Cargo package as the center piece and using as much metadata from the unmodified Cargo setup as possible, with minor annotations required as package-metadata.

By optionally using the unstable Cargo bindeps feature, we can even build multiple shared libraries from a single Cargo package, thus requiring no non-standard configuration to get exactly the artifacts needed for the respective bundle.

Unfortunately, we face 2 issues which we have to work around with rather ugly heuristics:

  1. Some OSs require the entry-point as shared object, others as executable. A common solution is to add a simple src/bin/runner.rs, which invokes the application. The build-system then either builds the executable or the library, depending on which type is needed for the OS. Unfortunately, this requires implementing some dummy for systems that don't need it (you cannot guard the binary based on CFGs), and will build unneeded artifacts, which might have to be filtered and identified as dummies.

    To avoid all this, we instead always build via cargo rustc --lib but pass --crate-type bin/cdylib depending on which output is needed for the target. This merely requires putting an entry-point fn main() into the library, which can be nicely guarded by cfgs and implemented exactly for the targets that need it.

    Unfortunately, Cargo does not report such executables as executable, which this PR tries to fix, so our build-system can easily identify the artifact. Otherwise, we have to guess which of the artifacts is the executable (which we currently do, but would like to avoid).

  2. Cargo can report a wide range of build-artifacts without any classification other than the executable key. We currently have to filter out files like .rmeta, .d, .a, ... to ensure we copy the right artifacts to the correct destination location. Ideally, we would tell Cargo which artifacts we want, and it would only produce those. But our current heuristic simply filters based on target-type/crate-type and file-extension. It works fine, but feels like it guesses information that Cargo already has.

    Unfortunately, executables are quite hard to filter, given they lack suitable file-extensions on many platforms (or might even have random file-extensions if the filename= override is used). Hence, we rely on the executable key to tell us about them. Without this, we have to guess it based on lack of known file-extension for each target-type/crate-type.

    Note that we already filter custom-build or proc-macro targets, so we would never copy their artifacts, even if executable is set. I just thought it would be a nice addition to report those as well.

Given the documentation of the executable key, I thought it was an oversight that it did not report executables in all scenarios. Hence, I filed this mostly as a bug-fix.

What exactly are your concerns? Is this about confusing existing tools which expect a single build to only ever report a single executable? I am not entirely sure how the key is used by external tools. We always parse the entire metadata before the build, so we can filter based on target-types, but I guess if you do not do this, it might be confusing to see multiple executable keys in the output.

Alternatively, I could reduce the PR to only report executables for --lib --crate-type=bin builds?

@epage
Copy link
Contributor

epage commented Mar 20, 2024

I feel there is a problem you are having that we need to dig into and better understand. Would you be willing to move this conversation to an Issue? I've found Issues are much better for discussions of problems and proposed solutions while PRs are better for the implementation. Part of this stems from PRs mixing problem/solution/implementation and part from the fact that there can be multiple PRs for one Problem and that can split the problem/solution discussion, making it harder to track.

@dvdhrm
Copy link
Contributor Author

dvdhrm commented Mar 21, 2024

Sure! Filed as #13612.

@weihanglo
Copy link
Member

Converted to draft for now until we have something from #13612.

@weihanglo weihanglo marked this pull request as draft March 22, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants