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

Add a doc.browser config option #9473

Merged
merged 5 commits into from
May 24, 2021
Merged

Add a doc.browser config option #9473

merged 5 commits into from
May 24, 2021

Conversation

lf-
Copy link
Contributor

@lf- lf- commented May 10, 2021

The idea of this option is to allow cargo to use a separate browser from
the rest of the system. My motivation in doing this is that I want to
write a script that adds a symbolic link in some web root on my system
such that I can access my docs via the http protocol to avoid the
limitations of the file protocol that are accessibility problems for me.
For instance, zoom is not retained across multiple pages and Stylus
filters don't work well.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2021
The idea of this option is to allow cargo to use a separate browser from
the rest of the system. My motivation in doing this is that I want to
write a script that adds a symbolic link in some web root on my system
such that I can access my docs via the http protocol to avoid the
limitations of the file protocol that are accessibility problems for me.
For instance, zoom is not retained across multiple pages and Stylus
filters don't work well.
src/cargo/ops/cargo_doc.rs Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented May 14, 2021

I think this seems reasonable. I had a concern about the config name. There is already a doc table, and I think it may be confusing to have both doc and cargo-doc. I think I would lean towards having a single table, and probably with the name doc, though I'd like to hear what the team says.

@rust-lang/cargo This proposes to add a new config variable to set the browser used with cargo doc. This can currently be set by the BROWSER environment variable, but this config will override that. I think it should be fine for this to be insta-stable, since it is such a small change.

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 14, 2021

Team member @ehuss has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels May 14, 2021
@ehuss
Copy link
Contributor

ehuss commented May 14, 2021

@rfcbot concern PathAndArgs

@ehuss
Copy link
Contributor

ehuss commented May 14, 2021

@rfcbot concern doc-vs-cargo-doc

@lf-
Copy link
Contributor Author

lf- commented May 14, 2021

I think this seems reasonable. I had a concern about the config name. There is already a doc table, and I think it may be confusing to have both doc and cargo-doc. I think I would lean towards having a single table, and probably with the name doc, though I'd like to hear what the team says.

I wasn't aware. I don't see one in the configuration reference in the docs, having looked just now. Is this part of some nightly option or missing from the docs?

I'm thinking this should maybe be PathAndArgs, so that there is some consistency with other config options that specify executables to run.

No disagreement with that from me, that would be a good idea. Should it also be applied to the existing BROWSER environment variable?

@ehuss
Copy link
Contributor

ehuss commented May 14, 2021

Is this part of some nightly option

Yea, it is currently unstable, documented here: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#rustdoc-map

Should it also be applied to the existing BROWSER environment variable?

Hm, I don't have much of an opinion here. I guess that is how $BROWSER usually works with xdg-open? (Though $BROWSER can also use $IFS, let's ignore that 😉 )

@lf-
Copy link
Contributor Author

lf- commented May 15, 2021

Alright, I've chosen not to do it to BROWSER also since it would introduce a regression on the handling of non unicode BROWSER values as I don't think I can split those at a space, so I'd have to convert it to Unicode lossily.

Previously we apparently weren't doing this as we weren't checking
stdout.
@ehuss
Copy link
Contributor

ehuss commented May 17, 2021

Seems fine, we can always tweak the BROWSER env var behavior in the future if needed.

@rfcbot resolve PathAndArgs

@ehuss
Copy link
Contributor

ehuss commented May 17, 2021

@rfcbot resolve doc-vs-cargo-doc

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels May 18, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented May 18, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

src/doc/src/reference/config.md Outdated Show resolved Hide resolved
src/doc/src/reference/environment-variables.md Outdated Show resolved Hide resolved
@ehuss
Copy link
Contributor

ehuss commented May 24, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2021

📌 Commit 6128b54 has been approved by ehuss

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 24, 2021
@bors
Copy link
Contributor

bors commented May 24, 2021

⌛ Testing commit 6128b54 with merge f898eff...

@bors
Copy link
Contributor

bors commented May 24, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing f898eff to master...

@bors bors merged commit f898eff into rust-lang:master May 24, 2021
@bors bors mentioned this pull request May 24, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
Update cargo

7 commits in 070e459c2d8b79c5b2ac5218064e7603329c92ae..e931e4796b61de593aa1097649445e535c9c7ee0
2021-05-11 18:12:23 +0000 to 2021-05-24 16:17:27 +0000
- Add `cargo:rustc-link-arg-bin` flag. (rust-lang/cargo#9486)
- Add a cargo-doc.browser config option (rust-lang/cargo#9473)
- Fix bug when with resolver = "1" non-virtual package was allowing unknown features (rust-lang/cargo#9437)
- Add GitHub link to contributor guide. (rust-lang/cargo#9493)
- Add temporary fix for rustup on windows in CI. (rust-lang/cargo#9498)
- 3 typos and some capitalization (rust-lang/cargo#9495)
- fix 6 typos (rust-lang/cargo#9484)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request May 26, 2021
Update cargo

7 commits in 070e459c2d8b79c5b2ac5218064e7603329c92ae..e931e4796b61de593aa1097649445e535c9c7ee0
2021-05-11 18:12:23 +0000 to 2021-05-24 16:17:27 +0000
- Add `cargo:rustc-link-arg-bin` flag. (rust-lang/cargo#9486)
- Add a cargo-doc.browser config option (rust-lang/cargo#9473)
- Fix bug when with resolver = "1" non-virtual package was allowing unknown features (rust-lang/cargo#9437)
- Add GitHub link to contributor guide. (rust-lang/cargo#9493)
- Add temporary fix for rustup on windows in CI. (rust-lang/cargo#9498)
- 3 typos and some capitalization (rust-lang/cargo#9495)
- fix 6 typos (rust-lang/cargo#9484)
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels May 28, 2021
@ehuss ehuss changed the title Add a cargo-doc.browser config option Add a doc.browser config option Jun 18, 2021
@ehuss ehuss added this to the 1.54.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants