Store feature flags per release in database #1129

almusil · 2020-10-27T10:51:22Z

Hello,

I have attempted to implement feature requested in #590. I am not sure if I have managed to find every reference correctly.

jyn514

Thank you so much for working on this!

src/db/migrate.rs

templates/rustdoc/topbar.html

jyn514 · 2020-10-27T15:29:52Z

templates/rustdoc/topbar.html

+    </li>{#
+    Display the features available in current build
+    #}
+    {% if 'krate' in __tera_context %}


What does this do? I would prefer to use {% if krate.features %} or things like that instead, that will also avoid bugs like #1111.

Also, I think this should be shown on every page that has Platform, there's no reason to special case the documentation page. You may need to add information about features to other pages, let me know if you need help with that.

The first one is done. I am not sure how to address the second one. Can you please give me a hint?

jyn514 · 2020-10-27T15:34:44Z

src/web/crate_details.rs

@@ -187,6 +189,7 @@ impl CrateDetails {
            total_items: total_items.map(|v| v as f32),
            total_items_needing_examples: total_items_needing_examples.map(|v| v as f32),
            items_with_examples: items_with_examples.map(|v| v as f32),
+            features: krate.get("features"),


We should also strip out 'private' features with underscores and optional dependencies. See some of the discussion in https://internals.rust-lang.org/t/docs-rs-crate-features-display/12895/3.

Added, hopefully I did understand the requirements.

jyn514 · 2020-10-27T15:36:53Z

Screenshots:

jyn514 · 2020-10-27T16:43:34Z

Also, this currently panics on crates built before the feature was added:

2020/10/27 12:39:39 [INFO] docs_rs::web: Running docs.rs web server on http://0.0.0.0:3000
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: Msg("Failed to render \'rustdoc/topbar.html\'"), source: Some(Error { kind: Msg("Tried to iterate on a container (`krate.features`) that has a unsupported type"), source: None }) }', src/utils/html.rs:24:77
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Could you add a test for that?

almusil · 2020-10-28T10:08:11Z

I have added two test cases but I am not sure where can I test the add_release.rs part. Can you please help me out?

jyn514

I am not sure where can I test the add_release.rs part

Do you mean how to test documentation for an old crate? You'll have to insert a crate into the database which has NULL for features. I think the easiest way would be to make features an Option in tests only (with cfg(test)). Then you could explicitly set it to None for the test.

I am not sure how to address the second one.

krate is provided by WebPage for rustdoc pages here:

docs.rs/src/web/rustdoc.rs

Lines 437 to 446 in 750061b

    
           RustdocPage { 
        
               latest_path, 
        
               latest_version, 
        
               target, 
        
               inner_path, 
        
               is_latest_version, 
        
               is_prerelease, 
        
               metadata: krate.metadata.clone(), 
        
               krate, 
        
           }

But on /crate it's called details instead:

docs.rs/src/web/crate_details.rs

Line 302 in 750061b

CrateDetailsPage { details }.into_response(req)

I think you'll need to rename them both to be consistent (I like krate better than details) so you can use them both in the header. Otherwise, you'll only ever have krate or details but not both and it will be annoying to work with.

jyn514 · 2020-10-28T13:57:57Z

src/db/add_package.rs

+    features.extend(
+        pkg.features
+            .iter()
+            .filter(|(name, _)| !name.eq(&"default"))


I think this can be simplified.

Suggested change

.filter(|(name, _)| !name.eq(&"default"))

.filter(|(name, _)| name != "default")

jyn514 · 2020-10-28T13:59:57Z

src/db/add_package.rs

+        .filter(|dep| {
+            pkg.features
+                .values()
+                .any(|features| features.iter().any(|sub| sub.eq(&dep.name)))


The code can be simplified:

Suggested change

.any(|features| features.iter().any(|sub| sub.eq(&dep.name)))

.any(|features| features.contains(&dep.name))

But the logic doesn't look quite right - we want to show optional dependencies only if they do not match the name of an optional feature. Otherwise serde will show up twice in

[features] serde = [] [dependencies] serde = { version = "1", optional = true }

but optional dependencies without a feature won't show up at all:

[dependencies] ansi-color = { version = "1", optional = true }

Can you fix that and add a test case?

You can't have a feature with the same name as a dependency.

We probably want to not filter anything out when storing the features into the database, and instead filter them during display so that we can change the heuristics and have them apply to existing data.

jyn514 · 2020-10-28T14:04:50Z

src/test/fakes.rs

+                .iter()
+                .cloned()


😢 rust-lang/rust#65819

jyn514 · 2020-10-28T14:11:03Z

src/web/crate_details.rs

+    features.map(|vec| {
+        vec.iter()
+            .filter(|feature| !feature.is_private())
+            .map(|feature| format!("{}", feature))


Suggested change

.map(|feature| format!("{}", feature))

.map(|feature| feature.to_string())

jyn514 · 2020-10-28T14:12:38Z

src/web/crate_details.rs

+            let features = [("_private".into(), Vec::new())]
+                .iter()
+                .cloned()
+                .collect::<HashMap<String, Vec<String>>>();


You use this later, so I think the type annotation can be removed.

Suggested change

.collect::<HashMap<String, Vec<String>>>();

.collect()

jyn514 · 2020-10-28T14:13:25Z

templates/rustdoc/topbar.html

+    </li>{#
+    Display the features available in current build
+    #}
+    {% if krate.features %}


You need {%- to remove the whitespace (which @Nemo157 said broke things for some reason).

Suggested change

{% if krate.features %}

{%- if krate.features -%}

Not breakage, just an extra space gets rendered making the spacing between elements in the header inconsistent 😁

Nemo157 · 2020-10-28T15:00:26Z

I think you'll need to rename them both to be consistent (I like krate better than details) so you can use them both in the header. Otherwise, you'll only ever have krate or details but not both and it will be annoying to work with.

The header (other than the crate details dropdown) uses metadata for the values, which is populated on all the /crate/.../ subpages, so you would want to put the features on that instead of CrateDetails (and update the other pages to pull the details from the database too)

docs.rs/src/web/mod.rs

Line 513 in 7109f40

pub(crate) struct MetaData {

(EDIT: Related #1138, to simplify and unify the handling of this across all pages that show the per-crate navbar elements).

jyn514 · 2020-10-28T17:35:33Z

If possible it would be nice to also make the display look more like rust-search-extension: https://rust.extension.sh/feature-flags.png. In particular I like making the feature look the same as doc(cfg) with the blue border, and the alignment of the equal signs. But we can fix those in follow-ups, they don't have to block the initial implementation.

Kixiron · 2020-10-28T20:38:20Z

I'm not really sold on this layout, it looks super mobile-unfriendly and a wee claustrophobic. I think it'd be better as a page alongside the builds and crate page, with a link on the top bar going to it. I'm also not really a fan of the "toml dump" it has currently (the key = ["value"]), I'd rather it involve tables or something along those lines so things look pretty

jyn514 · 2020-10-28T20:45:55Z

@Kixiron sure, if you want we can make this just collect the features in the database and we can work out the frontend later, like we did for #1100 . But I'd like to start collecting features quickly since otherwise we'll have to rebuild crates for the data to show up.

Kixiron · 2020-10-28T20:48:23Z

Sounds good to me, design is slow

almusil · 2020-10-29T07:18:45Z

@Kixiron sure, if you want we can make this just collect the features in the database and we can work out the frontend later, like we did for #1100 . But I'd like to start collecting features quickly since otherwise we'll have to rebuild crates for the data to show up.

@jyn514 So should I skip the frontend part entirely and just put the features into database?

jyn514 · 2020-10-29T13:41:41Z

@almusil for now, let's do that. If you want to avoid throwing away your work you could make a follow-up PR with the design changes and we can bikeshed that.

almusil · 2020-10-29T13:58:32Z

@almusil for now, let's do that. If you want to avoid throwing away your work you could make a follow-up PR with the design changes and we can bikeshed that.

Ok, I will create additional WIP PR with the design that was here to continue it. Thanks

jyn514

LGTM with nit fixed

jyn514 · 2020-10-29T13:56:16Z

src/db/add_package.rs

+    features.extend(
+        pkg.features
+            .iter()
+            .filter(|(name, _)| name != "default")


Suggested change

.filter(|(name, _)| name != "default")

.filter(|(name, _)| *name != "default")

src/db/add_package.rs#L229 can't compare `&std::string::String` with `str`

jyn514 · 2020-10-29T13:59:13Z

src/db/add_package.rs

+    features
+}
+
+fn get_optional_features(pkg: &MetadataPackage) -> Vec<Feature> {


This is getting optional dependencies - can you put that in the name?

Suggested change

fn get_optional_features(pkg: &MetadataPackage) -> Vec<Feature> {

fn get_optional_dependencies(pkg: &MetadataPackage) -> Vec<Feature> {

Feature flags should show all available features for current crate version with default being first is specified.

almusil force-pushed the add_feature_menu branch 4 times, most recently from 8b1addf to e17b6e0 Compare October 27, 2020 12:08

jyn514 added A-builds Area: Building the documentation for a crate A-frontend Area: Web frontend C-enhancement Category: This is a new feature S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 27, 2020

jyn514 reviewed Oct 27, 2020

View reviewed changes

jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 27, 2020

almusil force-pushed the add_feature_menu branch 2 times, most recently from 2ebcdd5 to cc4883b Compare October 28, 2020 10:02

jyn514 reviewed Oct 28, 2020

View reviewed changes

almusil force-pushed the add_feature_menu branch from cc4883b to 1087361 Compare October 29, 2020 13:50

almusil changed the title ~~Add menu item called Feature flags~~ Store feature flags per release in database Oct 29, 2020

almusil force-pushed the add_feature_menu branch from 1087361 to c9ce90a Compare October 29, 2020 13:56

jyn514 reviewed Oct 29, 2020

View reviewed changes

Add menu item called Feature flags

75fa1c5

Feature flags should show all available features for current crate version with default being first is specified.

almusil force-pushed the add_feature_menu branch from c9ce90a to 75fa1c5 Compare October 29, 2020 14:03

jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed A-frontend Area: Web frontend S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 29, 2020

jyn514 merged commit f6fd6cf into rust-lang:master Oct 29, 2020

almusil deleted the add_feature_menu branch October 29, 2020 14:11

almusil mentioned this pull request Oct 30, 2020

frontend: Show feature flags in topbar and separate page #1144

Merged

jyn514 removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store feature flags per release in database #1129

Store feature flags per release in database #1129

almusil commented Oct 27, 2020

jyn514 left a comment

jyn514 Oct 27, 2020

jyn514 Oct 27, 2020 •

edited

Loading

almusil Oct 28, 2020

jyn514 Oct 27, 2020

almusil Oct 28, 2020

jyn514 commented Oct 27, 2020

jyn514 commented Oct 27, 2020

almusil commented Oct 28, 2020

jyn514 left a comment

jyn514 Oct 28, 2020

jyn514 Oct 28, 2020

Nemo157 Oct 28, 2020

Nemo157 Oct 28, 2020

jyn514 Oct 28, 2020

jyn514 Oct 28, 2020

jyn514 Oct 28, 2020

jyn514 Oct 28, 2020

Nemo157 Oct 28, 2020

Nemo157 commented Oct 28, 2020 •

edited

Loading

jyn514 commented Oct 28, 2020

Kixiron commented Oct 28, 2020

jyn514 commented Oct 28, 2020

Kixiron commented Oct 28, 2020 •

edited

Loading

almusil commented Oct 29, 2020

jyn514 commented Oct 29, 2020

almusil commented Oct 29, 2020

jyn514 left a comment

jyn514 Oct 29, 2020

almusil Oct 29, 2020

jyn514 Oct 29, 2020

almusil Oct 29, 2020

	RustdocPage {
	latest_path,
	latest_version,
	target,
	inner_path,
	is_latest_version,
	is_prerelease,
	metadata: krate.metadata.clone(),
	krate,
	}

	.filter(\|(name, _)\| !name.eq(&"default"))
	.filter(\|(name, _)\| name != "default")

	.any(\|features\| features.iter().any(\|sub\| sub.eq(&dep.name)))
	.any(\|features\| features.contains(&dep.name))

	.map(\|feature\| format!("{}", feature))
	.map(\|feature\| feature.to_string())

	.filter(\|(name, _)\| name != "default")
	.filter(\|(name, _)\| *name != "default")

	fn get_optional_features(pkg: &MetadataPackage) -> Vec<Feature> {
	fn get_optional_dependencies(pkg: &MetadataPackage) -> Vec<Feature> {

Store feature flags per release in database #1129

Store feature flags per release in database #1129

Conversation

almusil commented Oct 27, 2020

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 commented Oct 27, 2020

jyn514 commented Oct 27, 2020

almusil commented Oct 28, 2020

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemo157 commented Oct 28, 2020 • edited Loading

jyn514 commented Oct 28, 2020

Kixiron commented Oct 28, 2020

jyn514 commented Oct 28, 2020

Kixiron commented Oct 28, 2020 • edited Loading

almusil commented Oct 29, 2020

jyn514 commented Oct 29, 2020

almusil commented Oct 29, 2020

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Oct 27, 2020 •

edited

Loading

Nemo157 commented Oct 28, 2020 •

edited

Loading

Kixiron commented Oct 28, 2020 •

edited

Loading