Update gir: Use docsrs attribute

[AaronErhardt]

Follow-up of gtk-rs/gir#1455

Unfortunately, this change affects quite a lot of files, but most changes are very similar.

[bilelmoussaoui]

The sys crates has to be updated as well https://github.com/gtk-rs/gtk-rs-core/blob/master/pango/sys/build.rs

[AaronErhardt]

Everything is up to date as far as I can tell. The word "dox" doesn't appear anywhere in the code anymore, but the problem with the CI seems to be that actions-rs does not pick up env variables which are now necessary for RUSTCFLAGS...

I'm trying rust-toolchain now instead, which is better anyway because actions-rs is not maintained anymore. Let's see if that works.

[AaronErhardt]

Seems like I needed RUSTFLAGS instead of RUSTDOCFLAGS in the end, because build scripts don't care about the latter.

Still a pango job is still failing, but only because --all-features enabled "dox" so far. Was it intended to not check pkgconfig in this job?

[sdroege]

Still a pango job is still failing, but only because --all-features enabled "dox" so far. Was it intended to not check pkgconfig in this job?

For doc build, no native C libraries should be required so that it can build (somewhat) on docs.rs. Is that what you mean?

[bilelmoussaoui]

The problem with pango is that upstream didn't bump the release to say 1.51 when adding APIs that are expected to land as part of 1.52 so we are a bit stuck. The solution would be to open an issue upstream asking them to bump the api version to 1.51 for now or we could just test with v1_50 only for now.

[AaronErhardt]

Btw. to sort out the remaining issues, I'm waiting for gtk-rs/gir#1457 to land. This would be the only proper solution for the problem.

[sdroege]

Yes, makes sense to me. That will just require updating a lot of conditions in manual code too here now. The gir PR is merged in any case.

@bilelmoussaoui You'll have to update in gtk4-rs too.

[AaronErhardt]

Am I wrong or was this logic flawed? https://github.com/gtk-rs/gtk-rs-core/blob/master/gio/src/socket.rs#L880-L914

I think it should have been #[cfg(any(not(unix), feature = "dox"))] not #[cfg(all(not(unix), feature = "dox"))]. This would mean that this feature was documented on supported platforms, but never compiled (because you wouldn't use "dox" for regular builds), right?

[sdroege]

Am I wrong or was this logic flawed?

No that seems correct to me. When generating docs it is defining fallback types for the ones in std as those only exist either on Windows or on UNIXy platforms.

[AaronErhardt]

When generating docs it is defining fallback types for the ones in std as those only exist either on Windows or on UNIXy platforms.

Ok that makes sense. It looked like dead code to me, but if it's about a fallback for documentation, it sounds reasonable. It's still very unlikely that someone uses something that's neither Unix nor Windows for building docs, but at least I matched the old behavior with the latest commit.

[AaronErhardt]

I would say this should be ready now, but given the large size of the PR and how much code it touches (even if it's mostly the same stuff) I would like to get better feedback from the CI. Should I just temporarily disable the pango test case so the rest of the crates gets proper testing?

[bilelmoussaoui]

Should I just temporarily disable the pango test case so the rest of the crates gets proper testing?

As I mentioned, you can build it with v1_50 instead?

[bilelmoussaoui]

We can always fix pango upstream but as the maintainer is away for a few days, i guess it can't happen anytime soon

[sdroege]

It's still very unlikely that someone uses something that's neither Unix nor Windows for building docs, but at least I matched the old behavior with the latest commit.

That's not the reason. If you build on Windows you don't have UNIXy APIs in std, if you build on Linux you don't have Windows APIs in std. For that reason the Windows APIs are defined when building docs on Linux and vice-versa.

[bilelmoussaoui]

pango should be fixed once https://gitlab.gnome.org/GNOME/pango/-/merge_requests/691 lands

[bilelmoussaoui]

pango PR got merged, #1087 is the only missing piece.

[sdroege]

@bilelmoussaoui You rebase this one?

[sdroege]

I guess not :) I've rebased this one now. Let's see if the CI passes this time.

[sdroege]

Ah I can't

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access. Cannot force-push to this branch
To github.com:AaronErhardt/gtk-rs-core.git
 ! [remote rejected]         HEAD -> master (protected branch hook declined)

@AaronErhardt your job then :)

[sdroege]

@bilelmoussaoui Still fails :)

[AaronErhardt]

Everything except "docs embed check" seems to work now. Not sure why that one fails though.

[pentamassiv]

Are the changes in cairo/src/svg.rs, cairo/src/pdf.rs, gio/src/lib.rs and gio/src/socket.rs correct?

For example I would have expected [cfg(all(feature = "pdf", feature = "v1_16"))] instead of [cfg(any(all(feature = "pdf", feature = "v1_16"), docsrs))].

Looks like the features are not independent from the docsrs attribute (gtk-rs/gir#1457). The lines where I have my doubts can quickly be found by searching for docsrs)

[AaronErhardt]

@pentamassiv I think you are correct. There were so many attributes that needed to be changed because the dox feature was used heavily and often with some additional logic. This makes it likely that there were a couple of oversights from my side, like those you pointed out.

[pentamassiv]

No worries, I just wanted to ask in case I misunderstood something. If you'd like, I can create a PR to fix those leftovers

[AaronErhardt]

If you'd like, I can create a PR to fix those leftovers

That'd be great!