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

cargo doc --open always respect request_kind #11804

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

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Mar 6, 2023

What does this PR try to resolve?

closes #11728

More details:

The open behavior is as follows:
cargo doc --open:

  • Pick the first root unit that was built for host.
  • If none found, pick the first one(whatever it's target is).

cargo doc --target TARGET --open:

  • Pick the first root unit for the given target.
  • If none found, pick the first one(whatever it's target is).

How should we test and review this PR?

See the unit test.

@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added Command-doc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2023
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR still working in the process. I'll add a test later.

src/cargo/ops/cargo_doc.rs Outdated Show resolved Hide resolved
src/cargo/ops/cargo_doc.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Mar 6, 2023

My suggestion would be to have the following behavior:

  • cargo doc --open:

    Pick the first root unit that was built for host.
    If none found, pick the first one.

  • cargo doc --target TARGET --open:

    Pick the first root unit for the given target.
    If none found, pick the first one.

This may require extending the Compilation struct to include the necessary information. I would suggest iterating over the root units, filtering out the ones for documentation, and storing the required information (the name and kind).

@rustbot rustbot added the A-build-execution Area: anything dealing with executing the compiler label Mar 7, 2023
@Rustin170506
Copy link
Member Author

  • cargo doc --open:
    Pick the first root unit that was built for host.
    If none found, pick the first one.

How does it interact with forced-target?
My thought is:

  1. if the root unit has forced-target, then open the docs of the first root unit built for forced-target.
  2. If there is no forced-target, then open the docs of the first root unit built for the host.
  • cargo doc --target TARGET --open:
    Pick the first root unit for the given target.
    If none found, pick the first one.
  1. If the first root unit has forced-target and it same with --target TARGET, then he docs of the first root unit built for forced-target.
  2. If it is conflict with --target TARGET bail out an error for it.
  3. If there is no forced-target, then open the docs of the first root unit built for the TARGET.

@ehuss
Copy link
Contributor

ehuss commented Mar 7, 2023

How does it interact with forced-target?

It depends, but for the most part the code should not know or care about forced-target. If there is only one root unit, then that's the one that gets opened (whether it is forced-target or not). If there are multiple, such as in a workspace, and one of those is forced-target and the other is not, then it will open the one that is not forced-target.

As for the "first" entry, that is completely arbitrary, and is currently alphabetical I believe.

The operation should be relatively simple, I think it would be something like:

root_units.iter().find(|unit| unit.kind == requested_kind).next().unwrap_or(root_units[0]);

That's a very rough sketch, but the general shape of what I think it should look like.

@Rustin170506 Rustin170506 changed the title cargo doc --open respect forced-target cargo doc --open always respect request_kind Mar 8, 2023
@Rustin170506
Copy link
Member Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2023
Copy link
Member Author

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self check

@Rustin170506 Rustin170506 marked this pull request as ready for review March 9, 2023 03:12
@Rustin170506
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Mar 9, 2023
@Rustin170506
Copy link
Member Author

@ehuss Do you have any suggestion about which target I should use in the test on the stable channel? Thanks!

@ehuss
Copy link
Contributor

ehuss commented Mar 10, 2023

The cargo testsuite has various things to help with cross-compiling. At the start of the function, write:

    if cross_compile::disabled() {
        return;
    }

Then, for the target value, use cross_compile::alternate().

tests/testsuite/doc.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

The cargo testsuite has various things to help with cross-compiling. At the start of the function, write:

    if cross_compile::disabled() {
        return;
    }

Then, for the target value, use cross_compile::alternate().

Thanks for your help! It works.

@Rustin170506 Rustin170506 requested a review from ehuss March 10, 2023 07:08
@dramforever
Copy link

Hi, just to confirm, cargo doc -p has-forced-target --open falls into the cargo doc --open case and would find the (only?) target has-forced-target and open its docs with the correct triple, is this right?

tests/testsuite/doc.rs Outdated Show resolved Hide resolved
tests/testsuite/doc.rs Outdated Show resolved Hide resolved
tests/testsuite/doc.rs Outdated Show resolved Hide resolved
tests/testsuite/doc.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented Mar 11, 2023

Hi, just to confirm, cargo doc -p has-forced-target --open falls into the cargo doc --open case and would find the (only?) target has-forced-target and open its docs with the correct triple, is this right?

Yes.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-doc-open branch 2 times, most recently from 2f31cd6 to 7f90e71 Compare March 12, 2023 03:35
@Rustin170506
Copy link
Member Author

Rustin170506 commented Mar 12, 2023

Hi, just to confirm, cargo doc -p has-forced-target --open falls into the cargo doc --open case and would find the (only?) target has-forced-target and open its docs with the correct triple, is this right?

Added doc_and_open_virtual_manifest_one_project test for it.

@Rustin170506 Rustin170506 requested a review from ehuss March 12, 2023 03:42
@ehuss
Copy link
Contributor

ehuss commented Mar 14, 2023

In thinking about this more, I came up with a concern with this approach, to be followed up at #11728 (comment).

@ehuss
Copy link
Contributor

ehuss commented Mar 14, 2023

@rustbot blocked

@rustbot rustbot added S-blocked and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2023
@Rustin170506
Copy link
Member Author

@ehuss What should I do to help this PR move forward? Do you think we still need to wait for more discussion about this behavior?

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-blocked labels Apr 18, 2023
@weihanglo weihanglo marked this pull request as draft April 18, 2023 21:52
@bors
Copy link
Contributor

bors commented Jan 11, 2024

☔ The latest upstream changes (presumably #12252) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-execution Area: anything dealing with executing the compiler Command-doc S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'cargo doc --open' seemingly does nothing if target is different from forced-target
6 participants