chore: MSRV 1.63 #163

fundon · 2024-10-06T13:02:22Z

The MSRV supported by chrono and image is too low, resulting in compilation failure.

chrono: no need to lock version
image: MSRV 1.63
- disable default features on dependencies
- enabel some feature for testing on dev-dependencies

@ajrcarey Thank you for your guidance.

ajrcarey · 2024-10-12T18:15:12Z

Again you've submitted a PR without any context or explanation as to what you're trying to do and why. I appreciate your work, but I really need to understand the motivation behind the change.

I see you closed your last PR without actually answering any of the questions I posed. I'm disappointed by that.

fundon · 2024-10-12T20:34:04Z

I'm sorry for the lack of clarity.
I am integrating pdfium-render into our App and found that the MSRV supported by chrono and image is too low, resulting in compilation failure, so I made adjustments to this.

It was also found that the image library, if default-features were turned off, did not need to be raised to 1.70, and that the test cases did not use all features

fundon · 2024-10-12T21:32:14Z

If we do not lock the version and use the default features of image, its dependencies may require 1.70, so 1.63 is a minimal adjustment when features are not required.

frederikhors · 2024-10-24T13:09:17Z

@ajrcarey this PR fixes a lot of issues we're having on our project. Please can you merge?

frederikhors · 2024-10-24T13:53:36Z

@fundon I think we can upgrade image to 0.25 too.

ajrcarey · 2024-10-27T20:58:50Z

Cargo.toml

 itertools = "0"
 log = "0"
 maybe-owned = "0"
 once_cell = "1"
 utf16string = "0"
 vecmath = "1"
+image = { version = "0.24", default-features = false, optional = true }


Why 0.24 and not >= 0.24? The latter allows for using image 0.25 - does your change? What is the impact of default-features = false here?

0.24 := >=0.24.0, <0.25.0

image 0.24 requires msrv 1.63, image 0.25 requires msrv 1.67.1

$ cargo msrv Fetching index Determining the Minimum Supported Rust Version (MSRV) for toolchain aarch64-apple-darwin Using check command cargo check Check for toolchain '1.57.0-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: failed to parse manifest at `/Users/fundon/dev/pdfium-render/Cargo.toml` │ │ │ │ Caused by: │ │ namespaced features with the `dep:` prefix are only allowed on the nightly channel and requires the `-Z namespaced-features` flag on the │ │ command-line │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Check for toolchain '1.70.0-aarch64-apple-darwin' succeeded Check for toolchain '1.63.0-aarch64-apple-darwin' succeeded Check for toolchain '1.60.0-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: package `image v0.24.9` cannot be built because it requires rustc 1.63.0 or newer, while the currently active rustc version is 1.60.0 │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Check for toolchain '1.61.0-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: package `image v0.24.9` cannot be built because it requires rustc 1.63.0 or newer, while the currently active rustc version is 1.61.0 │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Check for toolchain '1.62.1-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: package `image v0.24.9` cannot be built because it requires rustc 1.63.0 or newer, while the currently active rustc version is 1.62.1 │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

$ cargo msrv Fetching index Determining the Minimum Supported Rust Version (MSRV) for toolchain aarch64-apple-darwin Using check command cargo check Check for toolchain '1.57.0-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: failed to parse manifest at `/Users/fundon/dev/pdfium-render/Cargo.toml` │ │ │ │ Caused by: │ │ namespaced features with the `dep:` prefix are only allowed on the nightly channel and requires the `-Z namespaced-features` flag on the │ │ command-line │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Check for toolchain '1.70.0-aarch64-apple-darwin' succeeded Check for toolchain '1.63.0-aarch64-apple-darwin' failed with: ┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ error: package `image v0.25.4` cannot be built because it requires rustc 1.67.1 or newer, while the currently active rustc version is 1.63.0 │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ Unable to install toolchain with `rustup install 1.66.1-aarch64-apple-darwin`.

some default features of image are not used, so they can be turned off

ajrcarey · 2024-10-27T20:59:58Z

Cargo.toml

+  "png",
+  "jpeg",
+  "webp",
+] }


Is there an advantage to including the features list here, rather than at line 33?

Several image format features are used in the test case, which can be enabled on demand to keep it simple.

ajrcarey · 2024-10-27T21:01:09Z

Cargo.toml

@@ -54,14 +54,19 @@ windows = { version = "0", optional = true }

 [build-dependencies]
 # Bindgen 0.70.0 and later cause build failures when compiling to WASM. For more details, see:
-# 
+#


I think there was meant to be a link to #156 here. I'll add it separately.

ajrcarey · 2024-10-27T21:01:34Z

Cargo.toml

@@ -2,7 +2,7 @@
 name = "pdfium-render"
 version = "0.8.25"
 edition = "2018"
-rust-version = "1.60"
+rust-version = "1.63"


This change breaks the published MSRV for this crate. You need to clearly and persuasively explain why you are doing this.

Just like the problem encountered by the above image.

ajrcarey · 2024-10-27T21:04:37Z

Apologies for the delay; I have been rather ill. I am still not fully recovered, so there will be a further delay.

As this crate approaches its release candidate for 1.0, I am increasingly reluctant to merge PRs that make changes to dependencies without a clear justification as to the motivation. Changing dependencies at this point could have a detrimental impact on other consumers, so the PR needs to be persuasive as to why the change is necessary.

Neither this PR nor your previous one meet that bar.

The MSRV supported by chrono and image is too low, resulting in compilation failure.

What compilation failures? What leads you to the conclusion that compilation failures in your project are caused by pdfium-render? (I'm not saying they're not, but I want to understand exactly how.) What does "too low" mean?

When I asked you to explain the changes, you replied:

found that the MSRV supported by chrono and image is too low, resulting in compilation failure, so I made adjustments to this.

That's simply repeating what you first said!

I have added some comments to the PR itself at the specific points where the changes are not clear to me. I will not accept this PR until you clearly justify why the changes are necessary and why impact on other consumers is justified by your use case.

this PR fixes a lot of issues we're having on our project. Please can you merge?

Go into detail about your issues, please, and how this PR would fix them. How have you come to that conclusion? What tests have you run to verify that?

Generally speaking, I have some regrets about ever adding the image feature to this crate. It was initially convenient, but I should have forseen that latching pdfium-render to another dependency was inherently a bad idea. If I had the opportunity to do things over, the image feature would not exist. However, it's too late for that at this point.

One possibility might be to let crate consumers choose their own image version to consume, in a similar way to how the pdfium_* features allow consumers to select their Pdfium API version. By default, consumers bind to the latest supported Pdfium API version, but can override this to pin pdfium-render's API support to a specific version. Similarily, the existing image feature could default to the latest point release of the image crate, which I believe is currently 0.25. However, a consumer could explicitly select their version using new features image_024, image_025, etc. Once the image crate rolls over to 0.26 at some point in the future, the image feature of this crate would bump to that new version. This has the advantage of allowing consumers to pin their versions and maintain compatibility while still being able to upgrade pdfium-render to access bug fixes and new features.

I expect to be fully recovered by next weekend. If you have time to respond to the points I've raised by then, we may be able to get this moving again.

frederikhors · 2024-10-28T11:21:30Z

@ajrcarey I really appreciate your attention to detail and concern for any user issues, but I prefer to update even if it means losing a few minutes to fix the issues that update introduces. And I think that those who don't want to update don't even deserve amazing projects like this.

This is the error if I add pdfium-render = { version = "0.8.25" } to my root Cargo.toml:

cargo-clif test --workspace
    Updating crates.io index
error: failed to select a version for `chrono`.
    ... required by package `pdfium-render v0.8.25`
    ... which satisfies dependency `pdfium-render = "^0.8.25"` of package `players v0.1.0 (C:\project\src\players)`
    ... which satisfies path dependency `players` (locked to 0.1.0) of package `app v0.1.0 (C:\project\src\app)`
versions that meet the requirements `^0.4, <=0.4.31` are: 0.4.31, 0.4.30, 0.4.29, 0.4.28, 0.4.27, 0.4.26, 0.4.25, 0.4.24, 0.4.23, 0.4.22, 0.4.21, 0.4.20, 0.4.19, 0.4.18, 0.4.17, 0.4.16, 0.4.15, 0.4.13, 0.4.12, 0.4.11, 0.4.10, 0.4.9, 0.4.8, 0.4.7, 0.4.6, 0.4.5, 0.4.4, 0.4.3, 0.4.2, 0.4.1, 0.4.0

all possible versions conflict with previously selected packages.

  previously selected package `chrono v0.4.38`
    ... which satisfies dependency `chrono = "=0.4.38"` of package `demo v0.1.0 (C:\project\src\demo)`

failed to select a version for `chrono` which could resolve this conflict

In my project I'm using chrono = { version = "=0.4.38" }.

The same happens for image crate: I'm using an higher version.

For now I solved it by using a local version of your wonderful project and updating the libraries and it works!

cristicbz · 2024-10-28T14:05:55Z

Just to add another related failure this causes: in our case, we're using a workspace and getting a failure due to a different, unrelaated dependency of chrono in the same workspace. <= and = dependencies just don't work well with workspaces, and if semver is respected they shouldn't be necessary (rust-lang/cargo#13594).

Here's a minimum reproducer:

$ cat Cargo.toml
[workspace]
members = [ "bar","foo"]
resolver = "2"

[workspace.dependencies]
pdfium-render = { version = "0.8.25", features = ["libstdc++", "static", "thread_safe"] }
chrono = { version = "0.4.38" }

$ cat foo/Cargo.toml
[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
pdfium-render.workspace = true

$ cat bar/Cargo.toml
[package]
name = "bar"
version = "0.1.0"
edition = "2021"

[dependencies]
chrono.workspace = true

$ cargo build
    Updating crates.io index
error: failed to select a version for `chrono`.
    ... required by package `pdfium-render v0.8.25`
    ... which satisfies dependency `pdfium-render = "^0.8.25"` of package `foo v0.1.0 (/home/cristi/r/check/foo)`
versions that meet the requirements `^0.4, <=0.4.31` are: 0.4.31, 0.4.30, 0.4.29, 0.4.28, 0.4.27, 0.4.26, 0.4.25, 0.4.24, 0.4.23, 0.4.22, 0.4.21, 0.4.20, 0.4.19, 0.4.18, 0.4.17, 0.4.16, 0.4.15, 0.4.13, 0.4.12, 0.4.11, 0.4.10, 0.4.9, 0.4.8, 0.4.7, 0.4.6, 0.4.5, 0.4.4, 0.4.3, 0.4.2, 0.4.1, 0.4.0

all possible versions conflict with previously selected packages.

  previously selected package `chrono v0.4.38`
    ... which satisfies dependency `chrono = "^0.4.38"` of package `bar v0.1.0 (/home/cristi/r/check/bar)`

failed to select a version for `chrono` which could resolve this conflict

fundon · 2024-11-02T13:55:02Z

Apologies for the delay; I have been rather ill. I am still not fully recovered, so there will be a further delay.

I wish you a speedy recovery. Thanks for reply.

ajrcarey · 2024-11-03T16:15:27Z

Ok, thank you for all the comments. The impression I have is that the chrono dependency is the key problem. Assuming that's correct, then I propose the following:

Adjust the chrono dependency from its current chrono = "0.4, <= 0.4.31" to simply chrono = "0.4" to allow it to float. This will necessarily relax the MSRV of this crate from 1.60 to 1.63 or similar.
Introduce new crate features for pinning the image crate version the consumer wishes to use. The image feature will default to the latest version, but a consumer can deliberately pin to an older version if they wish. This will see the default image dependency of this crate switch from version = "0.24" to version = "0.25".

These two changes will be implemented in the following issues respectively:

chrono dependency: Relax chrono dependency version based on consumer feedback. #166
image feature flags: Add new crate feature flags to allow consumers to pin their image version. #167

Feel free to comment there. Or, if you feel more changes need to made, please continue to comment against this PR. I do not (at this point) intend to merge the PR itself.

I aim to make these changes today. This gives you time to test them (by taking pdfium-render as a git dependency) before they are included in a new crate release.

ajrcarey · 2024-11-06T17:21:39Z

As there have been no further comments, I will shortly close this PR. I aim to release 0.8.26 this weekend, so do make comments against #166 and #167 if you have any problems.

frederikhors · 2024-11-06T17:23:25Z

@ajrcarey I haven't had time to try it, but I will try as soon as it's released

chore: MSRV 1.63

54ecd05

ajrcarey reviewed Oct 27, 2024

View reviewed changes

This was referenced Nov 3, 2024

Relax chrono dependency version based on consumer feedback. #166

Closed

Add new crate feature flags to allow consumers to pin their image version. #167

Closed

ajrcarey closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: MSRV 1.63 #163

chore: MSRV 1.63 #163

fundon commented Oct 6, 2024 •

edited

Loading

ajrcarey commented Oct 12, 2024

fundon commented Oct 12, 2024

fundon commented Oct 12, 2024

frederikhors commented Oct 24, 2024

frederikhors commented Oct 24, 2024

ajrcarey Oct 27, 2024

fundon Nov 2, 2024

ajrcarey Oct 27, 2024

fundon Nov 2, 2024

ajrcarey Oct 27, 2024

ajrcarey Oct 27, 2024

fundon Nov 2, 2024

ajrcarey commented Oct 27, 2024 •

edited

Loading

frederikhors commented Oct 28, 2024

cristicbz commented Oct 28, 2024 •

edited

Loading

fundon commented Nov 2, 2024

ajrcarey commented Nov 3, 2024

ajrcarey commented Nov 6, 2024

frederikhors commented Nov 6, 2024

chore: MSRV 1.63 #163

chore: MSRV 1.63 #163

Conversation

fundon commented Oct 6, 2024 • edited Loading

ajrcarey commented Oct 12, 2024

fundon commented Oct 12, 2024

fundon commented Oct 12, 2024

frederikhors commented Oct 24, 2024

frederikhors commented Oct 24, 2024

ajrcarey Oct 27, 2024

Choose a reason for hiding this comment

fundon Nov 2, 2024

Choose a reason for hiding this comment

ajrcarey Oct 27, 2024

Choose a reason for hiding this comment

fundon Nov 2, 2024

Choose a reason for hiding this comment

ajrcarey Oct 27, 2024

Choose a reason for hiding this comment

ajrcarey Oct 27, 2024

Choose a reason for hiding this comment

fundon Nov 2, 2024

Choose a reason for hiding this comment

ajrcarey commented Oct 27, 2024 • edited Loading

frederikhors commented Oct 28, 2024

cristicbz commented Oct 28, 2024 • edited Loading

fundon commented Nov 2, 2024

ajrcarey commented Nov 3, 2024

ajrcarey commented Nov 6, 2024

frederikhors commented Nov 6, 2024

fundon commented Oct 6, 2024 •

edited

Loading

ajrcarey commented Oct 27, 2024 •

edited

Loading

cristicbz commented Oct 28, 2024 •

edited

Loading