chore: MSRV 1.63 - Githubissues

fundon commented 1 month ago

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 commented 1 month ago

Hi @fundon,

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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

frederikhors commented 3 weeks ago

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

ajrcarey commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 (https://github.com/rust-lang/cargo/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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: https://github.com/ajrcarey/pdfium-render/issues/166
image feature flags: https://github.com/ajrcarey/pdfium-render/issues/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 commented 2 weeks ago

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 commented 2 weeks ago

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

ajrcarey / pdfium-render

chore: MSRV 1.63 #163