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

codegen/general: Emit "Since vXXX" in #[deprecated] attributes #1087

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

MarijnS95
Copy link
Contributor

This started as an effort to get rid of the [Deprecated since XXX] and # Deprecated in the documentation in favour of cleaner and more obvious:

image

Unfortunately that doesn't show unless the docs are compiled with at least that version - which only seems to be happening in GStreamer thus far.


Also emit the missing #[deprecated] notice for objects, right into the mod.rs file. Is it missing anywhere else?

TODO: Probably need the same deduplication for these attributes as we had for the features in #1080. Right now it shows multiple times, even though deprecation of a type should also hold for all members and functions. Unlike doc(cfg()) features (see #1080 for example) rustdoc does not seem to coalesce these:

image

Neither the module nor struct (wrapped inside `glib::wrapper`) had a
deprecation warning previously.
@sdroege sdroege requested a review from GuillaumeGomez March 30, 2021 07:04
@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

Unfortunately that doesn't show unless the docs are compiled with at least that version - which only seems to be happening in GStreamer thus far.

It would probably make sense to update the gtk-rs CI to use a docker image with latest GLib and everything else. That we don't build with the latest version seems suboptimal. Can you create an issue for that? For gtk4 this is already done anyway, maybe we can easily base it on that @bilelmoussaoui ?

@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

Probably need the same deduplication for these attributes as we had for the features in #1080. Right now it shows multiple times, even though deprecation of a type should also hold for all members and functions. Unlike doc(cfg()) features (see #1080 for example) rustdoc does not seem to coalesce these:

Not sure about this one. It's probably good to also have it next to each function? Then it's more obvious and not just when scrolling to the very top of the page. @GuillaumeGomez how is this handled in rustdoc and in other projects using this rustdoc feature?

@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

Unfortunately that doesn't show unless the docs are compiled with at least that version

Isn't that taken care of by the dox feature anyway?

@bilelmoussaoui
Copy link
Member

Unfortunately that doesn't show unless the docs are compiled with at least that version - which only seems to be happening in GStreamer thus far.

It would probably make sense to update the gtk-rs CI to use a docker image with latest GLib and everything else. That we don't build with the latest version seems suboptimal. Can you create an issue for that? For gtk4 this is already done anyway, maybe we can easily base it on that @bilelmoussaoui ?

There's already a docker image used for gtk-rs's CI. We can easily adapt it and build more stuff manually

@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

There's already a docker image used for gtk-rs's CI. We can easily adapt it and build more stuff manually

Can we easily get the whole latest GNOME platform in there?

@MarijnS95
Copy link
Contributor Author

That we don't build with the latest version seems suboptimal

Maybe some CI does somewhere, but there's no explicit dox = ["<latest feature version>", ...] like in GStreamer-rs.

Will have to see how that works out when #[cfg(docs)] becomes a thing though - should be possible to enable features when building docs from the cmdline with cargo doc I guess.

@MarijnS95
Copy link
Contributor Author

Not sure about this one. It's probably good to also have it next to each function? Then it's more obvious and not just when scrolling to the very top of the page. @GuillaumeGomez how is this handled in rustdoc and in other projects using this rustdoc feature?

As seen in #1080 this is filtered out for features already (and propagated, if a module has a feature scope it appears to be shown on all type pages under it), but it's complicated (hence not properly working I presume) for reexports.

Perhaps the same could be or will be done for deprecation warnings.

It is indeed a good point, when navigating straight to a function inside some type (link to anchor) it is easy to miss the deprecation warning on top.

@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

Maybe some CI does somewhere, but there's no explicit dox = ["<latest feature version>", ...] like in GStreamer-rs.

Maybe there should, @GuillaumeGomez? :)

@GuillaumeGomez
Copy link
Member

That we don't build with the latest version seems suboptimal

Maybe some CI does somewhere, but there's no explicit dox = ["<latest feature version>", ...] like in GStreamer-rs.

Do you mean like this https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/blob/master/gstreamer/Cargo.toml#L54 ? There is that in gtk-rs too. Which is why I didn't understand what you meant.

Will have to see how that works out when #[cfg(docs)] becomes a thing though - should be possible to enable features when building docs from the cmdline with cargo doc I guess.

There is cfg(doc) and cfg(doctest) already. The only problem being that it didn't work at all when we tried to use it with gtk-rs because of various problems and lack of support from cargo.

@MarijnS95
Copy link
Contributor Author

@GuillaumeGomez Exactly, it isn't there on the gdk nor gtk I was testing with, maybe the other crates have it?

There is cfg(doc) and cfg(doctest) already. The only problem being that it didn't work at all when we tried to use it with gtk-rs because of various problems and lack of support from cargo.

That's fine. As long as cfg(doc) can be used to automatically enable the latest version like we do now (instead of #[cfg(any(doc, feature = "some version"))], though that'd work as well) there is no issue here.

@bilelmoussaoui
Copy link
Member

There's already a docker image used for gtk-rs's CI. We can easily adapt it and build more stuff manually

Can we easily get the whole latest GNOME platform in there?

Using the container image published by gnome-build-meta and used at GNOME for CI would be possible I think. I will give it a try.

@sdroege
Copy link
Member

sdroege commented Mar 30, 2021

Using the container image published by gnome-build-meta and used at GNOME for CI would be possible I think. I will give it a try.

That would seem like a low-effort approach for getting the latest stable releases of everything for our CI here.

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez Exactly, it isn't there on the gdk nor gtk I was testing with, maybe the other crates have it?

There is cfg(doc) and cfg(doctest) already. The only problem being that it didn't work at all when we tried to use it with gtk-rs because of various problems and lack of support from cargo.

That's fine. As long as cfg(doc) can be used to automatically enable the latest version like we do now (instead of #[cfg(any(doc, feature = "some version"))], though that'd work as well) there is no issue here.

No, it's not fine at all. It was a huge setback for us because we had a lot of hopes into it and invested quite some time. Cargo still hasn't been fixed so we cannot make it work for the moment.

@MarijnS95
Copy link
Contributor Author

No, it's not fine at all. It was a huge setback for us because we had a lot of hopes into it and invested quite some time. Cargo still hasn't been fixed so we cannot make it work for the moment.

I understand your disappointment but that seems to have little to do with this PR adding extra context to #[deprecated = "The extra context"]?

The problem here is that we need to enable the latest feature when compiling the docs, otherwise things like #[cfg_attr(feature = "v3_16", deprecated)] don't show when the version isn't at least v3_16 while compiling the docs.

@MarijnS95
Copy link
Contributor Author

In other words, if the latest version feature can be added to the dox feature in gtk-rs like it is in gstreamer-rs we can remove the obnoxious [Deprecated since XXX] mention from the documentation in favour of just the attribute and the big red header in HTML rendering.

@GuillaumeGomez
Copy link
Member

What I'm saying is that for the moment, we should rely on the dox feature and not on cfg(doc).

@MarijnS95
Copy link
Contributor Author

What I'm saying is that for the moment, we should rely on the dox feature and not on cfg(doc).

But of course, we cannot switch to cfg(doc) if it doesn't work as noted in the PR (and iirc followup issue). That's a different change someone will have to make when it does work and rolls out to stable.

@MarijnS95
Copy link
Contributor Author

Anyway, can we go back to the discussion whether to add the latest version to the dox feature group/array like in GStreamer-rs, so that the docs are always built with the latest version in mind?

@GuillaumeGomez
Copy link
Member

For that there is no issue I can see.

@MarijnS95
Copy link
Contributor Author

For that there is no issue I can see.

#1009?

@sdroege sdroege merged commit 1959d1e into gtk-rs:master Apr 26, 2021
@MarijnS95 MarijnS95 deleted the deprecated-since branch April 26, 2021 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants