Work around docs.rs problem with cargo (ref #240)

[spadarian]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

At the moment, compiling the docs fail due to a problem with docs.rs/cargo. The full discussion is here: #240

This is a temporary fix for it. At least until docs.rs/cargo fixes the bug.

[spadarian]

@rmanoka
This temporary fix to the doc.rs problem works fine locally (it generates the docs) but it's failing CI for ubuntu-lts.

• GDAL version in LTS is 3.0
• Clippy complains that it cannot find some functions that are not available (e.g. gdal_sys::OSRGetAxesCount)
• The gdal functions calling those missing gdal_sys function are marked with #[cfg(all(major_ge_3, minor_ge_1))], so they should be ignored, right? I'm not sure how that works

[rmanoka]

The CI fails because feature docsrs you've added conflicts with our CI strategy which uses --all-features in one of the steps.

We could fix this, but it would be good to check if docs.rs have fixed their pipeline: their github explains how to run the build via docker locally. Could we check to see if we still have this issue? If the docs.rs pipeline is fixed, the next release should automatically fix the issue.

[spadarian]

We still have the problem.

[rmanoka]

The downside of our current approach is: users accidentally add this feature while building the code (and the resultant link errors are a bit dense to parse); however, I don't see any other immediate soln. Apologies for the delay, but I would like to hear from the other maintainers here.

[spadarian]

No problem. It is a workaround, so I understand that you need to hear from the other maintainers.

If it helps, I could add a panic if the feature is enabled and DOCS_RS is not set. When I add the feature to my crate, it shows a message like this:

$ cargo build
   Compiling gdal v0.12.0 (~/Downloads/georust/gdal)
error: failed to run custom build command for `gdal v0.12.0 (~/Downloads/georust/gdal)`

Caused by:
  process didn't exit successfully: `~/.cache/cargo/target/debug/build/gdal-27f15e9a798d0e24/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'The `docsrs` feature should not be enabled', ~/Downloads/georust/gdal/build.rs:6:9
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

We can add a reference to this PR or the issue as well.

[rmanoka]

Since the docs issue has been open for a while, let's go ahead with this approach.
@spadarian can you fix the CI to not use your new feature during testing?

@pka @jdroenner @michaelkirk any thoughts on merging this? Would this be a breaking change as it adds a new feature?

[spadarian]

I added the following message:

The `docsrs` feature should not be enabled.
  If you are using the `--all-features` flag, try replacing it with `--features bindgen,array`.
  See https://github.com/georust/gdal/pull/241 for more details.

That will be shown if the user enables the feature explicitly or by using the --all-features flag.

[michaelkirk]

I'm late to the discussion, so I'm sorry if I missed something, but another approach could be: Rather than trying to work-around the fact that we can't build on docs.rs with a "fake build", to instead get the real normal build to work for docs.rs

We'd need to remove the docs.rs "special handling" from the gdal crate and then install some extra dependencies into the docs.rs build container - in my testing I was able to get the docs build to succeed by installing the libgdal-dev package.

One downside would be that this would be using the version of gdal available on whatever version of ubuntu docs.rs is using. Currently that's gdal-2.6, whereas currently for the docs.rs build, it looks like we're faking that we have gdal 3.2 installed:

#[cfg(docsrs)]
pub fn gdal_version_info(_key: &str) -> String {
    "3020000".to_string()
}

Is that a deal breaker?

[spadarian]

How does that interact with markers such as #[cfg(all(major_ge_3, minor_ge_1))]?

Would those docs be compiled if the version is 2.6?

[rmanoka]

Great idea @michaelkirk ; tried it locally and it works. See #281

[spadarian]

That looks different from what @michaelkirk was saying, right @rmanoka?

Anyway, it works which is the important thing!

If I understand correctly, you are generating the code dynamically, so the code calling GDALVersionInfo doesn't even exist when DOCS_RS is not set... so there is no compilation error.

Well, we can close this PR and merge the other one!

[rmanoka]

If I understand correctly, you are generating the code dynamically, so the code calling GDALVersionInfo doesn't even exist when DOCS_RS is not set... so there is no compilation error.

Yes, we use an indirection from having a sys crate. I didn't realise the crates build env already includes libgdal which seems needed for the pkg-config in gdal-sys/build.rs

Well, we can close this PR and merge the other one!

Yes, I'm pushing for a crate release so we can be sure everything works. Will close if the other pr works.

[michaelkirk]

Yes, we use an indirection from having a sys crate. I didn't realise the crates build env already includes libgdal which seems needed for the pkg-config in gdal-sys/build.rs

I looked into this a bit, it seems like although the gdal package is installed by the docs.rs environment, it doesn't include the .pc file needed for pkg-config to function.

Interestingly though, the gdal-dev package does include the .pc file we need, but:

1. Although it worked when I installed the gdal-dev package interactively, when I actually tried to add it to the docsrs build system automation, there was a package conflict between a mariadb client and a mysqlclient package that I didn't yet find a solution to.

2. As @spadarian pointed out, this would tie the docs to an older version, so I think we wouldn't generate all the docs we want to generate.

In any case, it sounds like @rmanoka has a good work around in #281, so my vote would be to go with that.

[lnicola]

#281 got merged (right after the docs.rs bug got fixed), so this is hopefully no longer needed.