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

Replaces SEXPTYPE and Rboolean to the new enum-versions #742

Merged
merged 4 commits into from May 9, 2024

Conversation

CGMossa
Copy link
Member

@CGMossa CGMossa commented Apr 27, 2024

This relies on extendr/libR-sys#233

@CGMossa CGMossa marked this pull request as ready for review April 27, 2024 13:34
@CGMossa
Copy link
Member Author

CGMossa commented Apr 28, 2024

MSRV fails, before there is a package called home who has changed its MSRV

 cargo msrv  --path extendr-api --min 1.56.0
Fetching index
Determining the Minimum Supported Rust Version (MSRV) for toolchain aarch64-apple-darwin
Using check command cargo check

Check for toolchain '1.67.1-aarch64-apple-darwin' failed with:
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: package `home v0.5.9` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.67.1             │
│ Either upgrade to rustc 1.70.0 or newer, or use                                                                                                        │
│ cargo update -p home@0.5.9 --precise ver                                                                                                               │
│ where `ver` is the latest version of `home` supporting rustc 1.67.1                                                                                    │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.72.1-aarch64-apple-darwin' succeeded

Check for toolchain '1.69.0-aarch64-apple-darwin' failed with:
┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ error: package `home v0.5.9` cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.69.0             │
│ Either upgrade to rustc 1.70.0 or newer, or use                                                                                                        │
│ cargo update -p home@0.5.9 --precise ver                                                                                                               │
│ where `ver` is the latest version of `home` supporting rustc 1.69.0                                                                                    │
└────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
Check for toolchain '1.70.0-aarch64-apple-darwin' succeeded
   Finished The MSRV is: 1.70.0 

And here's the packages that relies on home:

home v0.5.9
└── which v4.4.2
    └── bindgen v0.69.4
        [build-dependencies]
        └── libR-sys v0.7.0 (https://github.com/extendr/libR-sys#ad5b6286)
            ├── extendr-api v0.6.0 (/Users/elea/Documents/GitHub/extendr-main/extendr-api)
            │   [dev-dependencies]
            │   └── extendr-macros v0.6.0 (proc-macro) (/Users/elea/Documents/GitHub/extendr-main/extendr-macros)
            │       └── extendr-api v0.6.0 (/Users/elea/Documents/GitHub/extendr-main/extendr-api) (*)
            └── extendr-engine v0.6.0 (/Users/elea/Documents/GitHub/extendr-main/extendr-engine)
                [dev-dependencies]
                ├── extendr-api v0.6.0 (/Users/elea/Documents/GitHub/extendr-main/extendr-api) (*)
                └── extendr-macros v0.6.0 (proc-macro) (/Users/elea/Documents/GitHub/extendr-main/extendr-macros) (*)
            [dev-dependencies]

Update: Actually, it is the which crate that has the MSRV of 1.70: https://github.com/harryfei/which-rs?tab=readme-ov-file#msrv

@eitsupi
Copy link
Contributor

eitsupi commented Apr 28, 2024

Since Rust on CRAN is not scheduled to be 1.70, packages that depend on extendr will not be accepted by CRAN if MSRV is set to 1.70.
If CRAN is a consideration, MSRV must be 1.69.0 or lower.

@Ilia-Kosenkov
Copy link
Member

What are you gonna do about MSRV issue?

@CGMossa
Copy link
Member Author

CGMossa commented May 9, 2024

What are you gonna do about MSRV issue?

I don't know. It is not caused by the code checked in here. So I wanted to merge these.. And then figure out what we do about bindgen changing their MSRV without a version change first.

@Ilia-Kosenkov
Copy link
Member

You then need to bump MSRV, at least temporarily. But yeah, this sucks

in bindgen, which uses `which`, and which
uses `home` which silently updated
their msrv to 1.70.
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't read n check them all, but looks good & checks pass so definitely a step in the right direction

@CGMossa
Copy link
Member Author

CGMossa commented May 9, 2024

You're absolutely right, I should have bumped the MSRV.

Will wait for CI and merge.

Thanks @Ilia-Kosenkov.

@CGMossa CGMossa merged commit 2a8fb6c into master May 9, 2024
15 checks passed
@CGMossa CGMossa deleted the sync_libr-sys branch May 9, 2024 08:23
@yutannihilation
Copy link
Contributor

Does choosing MSRV 1.70 mean extendr decided to stop supporting CRAN?

@CGMossa
Copy link
Member Author

CGMossa commented May 9, 2024

No, but I cannot do anything about this MSRV change. Even if I choose an older version of bindgen, I'm stuck with this.

I believe @JosiahParry will attempt a dialogue at increasing the available rust version on CRAN machine. In the mean time, I'm going to try to find a solution for this.

But we need to upgrade at some point, because I'd like to use GATS at some point in extendr.

Are you still of the opinion we should drop CRAN support? Because I'm personally getting very close to that opinion.

@yutannihilation
Copy link
Contributor

Are you still of the opinion we should drop CRAN support?

Of course? I actually take the drop as a good news :)
But, my point was that you shouldn't need to bump MSRV for an optional crate in theory.

@eitsupi
Copy link
Contributor

eitsupi commented May 9, 2024

Perhaps the CI error here was due to the lack of the Cargo.lock file and the bumping patch version of the dependency crate?

@CGMossa
Copy link
Member Author

CGMossa commented May 9, 2024

I don't know how the MSRV changed, but even if I locally, lower the bindgen version, and ensure that a cargo update happened, I still cannot get an MSRV of 1.67.

which-crate has this as repository: https://github.com/harryfei/which-rs
home-crate has this as its repository: https://github.com/rust-lang/cargo

Somehow, somewhere, the coupling has been disabled between SemVer, and bindgen. I have absolutely nothing I can do about it.

@eitsupi
Copy link
Contributor

eitsupi commented May 9, 2024

Since there is no lock file, you can only get the latest patch version, and as a result, you can't get the older version of the home crate?

@CGMossa
Copy link
Member Author

CGMossa commented May 9, 2024

I've been testing this locally. I believe locally, I've got a Cargo.lock. And I'm running cargo msrv --path extendr-api locally too. And those are failing.

Please, if you've got a hint as to how this is supposed to be interpreted, I'm all ears.

The recommendation is always, if you're making a CLI, then check-in Cargo.lock, and if it is a library, don't. So that's what we are following.

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.

None yet

4 participants