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

replace unmaintained yaml-rust with active fork #526

Closed
wants to merge 1 commit into from

Conversation

syphar
Copy link
Contributor

@syphar syphar commented Apr 2, 2024

see rustsec/advisory-db#1921 , I'm actually coming from rust-lang/docs.rs#2469.

When running the tests locally I'm seeing some test failures, probably related to the test setup, I assume that CI will do the necessary test setup and we'll see if this change is fine.

As a personal sidenote: not using rustfmt actually made constributing a little harder for me, it's so standard in my personal experience that it's in my editor / IDE config automatically on safe, which I needed to disable for this change.

@Enselic
Copy link
Collaborator

Enselic commented Apr 2, 2024

Even before the xz utils debacle, I would be skeptical of this change. I am not comfortable to change to relatively new crate with not that many downloads.

I agree with should rustfmt all code and enforce it in CI though. More or less all other projects in the Rust ecosystem does that, and the upsides far outweighs the downside IMHO.

Thank you for taking the time to create a PR, but I will go ahead and close this now given the current circumstances.

@Enselic Enselic closed this Apr 2, 2024
@syphar syphar deleted the replace-yaml branch April 2, 2024 06:07
@GuillaumeGomez
Copy link

Does it help if I know personally the maintainer of the new crate?

@Enselic
Copy link
Collaborator

Enselic commented Apr 2, 2024

Yes, that helps, thanks :)

I still feel it is too rushed to replace the crate in syntect. The crate we currently use has no problems as far as I know, so the least risky thing seems to be to keep everything as it is.

Once actual problems arise, the prio of doing this would increase of course.

@GuillaumeGomez
Copy link

Well, the API is the same so when some time has passed and your confidence in the crate grew, please consider it again. ;)

nazmulidris added a commit to r3bl-org/r3bl-open-core that referenced this pull request Apr 15, 2024
`yaml-rust` crate is unmaintained

1) `syntect` author won't update this dep to a fork of it due to lack
of trust concerns with this new fork:
trishume/syntect#526

2) cargo-deny produces this output:

error[unmaintained]: yaml-rust is unmaintained.
    ┌─ /home/nazmul/github/r3bl-open-core/Cargo.lock:295:1
    │
295 │ yaml-rust 0.4.5 registry+https://github.com/rust-lang/crates.io-index
    │ --------------------------------------------------------------------- unmaintained advisory detected
    │
    = ID: RUSTSEC-2024-0320
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0320
    = The maintainer seems [unreachable](chyh1990/yaml-rust#197).

      Many issues and pull requests have been submitted over the years
      without any [response](chyh1990/yaml-rust#160).

      ## Alternatives

      Consider switching to the actively maintained `yaml-rust2` fork of the original project:

      - [yaml-rust2](https://github.com/Ethiraric/yaml-rust2)
      - [yaml-rust2 @ crates.io](https://crates.io/crates/yaml-rust2))
    = Announcement: rustsec/advisory-db#1921
    = Solution: No safe upgrade is available!
    = yaml-rust v0.4.5
      └── syntect v5.1.0
          └── r3bl_tui v0.5.2
              └── r3bl-cmdr v0.0.11
nazmulidris added a commit to r3bl-org/r3bl-open-core that referenced this pull request Apr 15, 2024
`yaml-rust` crate is unmaintained

1) `syntect` author won't update this dep to a fork of it due to lack
of trust concerns with this new fork:
trishume/syntect#526

2) cargo-deny produces this output:

error[unmaintained]: yaml-rust is unmaintained.
    ┌─ /home/nazmul/github/r3bl-open-core/Cargo.lock:295:1
    │
295 │ yaml-rust 0.4.5 registry+https://github.com/rust-lang/crates.io-index
    │ --------------------------------------------------------------------- unmaintained advisory detected
    │
    = ID: RUSTSEC-2024-0320
    = Advisory: https://rustsec.org/advisories/RUSTSEC-2024-0320
    = The maintainer seems [unreachable](chyh1990/yaml-rust#197).

      Many issues and pull requests have been submitted over the years
      without any [response](chyh1990/yaml-rust#160).

      ## Alternatives

      Consider switching to the actively maintained `yaml-rust2` fork of the original project:

      - [yaml-rust2](https://github.com/Ethiraric/yaml-rust2)
      - [yaml-rust2 @ crates.io](https://crates.io/crates/yaml-rust2))
    = Announcement: rustsec/advisory-db#1921
    = Solution: No safe upgrade is available!
    = yaml-rust v0.4.5
      └── syntect v5.1.0
          └── r3bl_tui v0.5.2
              └── r3bl-cmdr v0.0.11
nazmulidris added a commit to r3bl-org/r3bl-open-core that referenced this pull request Apr 16, 2024
More info in this issue in the syntect repo:
trishume/syntect#526
@nazmulidris
Copy link

nazmulidris commented Apr 17, 2024

Yes, that helps, thanks :)

I still feel it is too rushed to replace the crate in syntect. The crate we currently use has no problems as far as I know, so the least risky thing seems to be to keep everything as it is.

Once actual problems arise, the prio of doing this would increase of course.

@Enselic Does it help (to mitigate the risk of using this crate) that other prolific crates have started using yaml-rust2 crate?

Here's another thread on merging the efforts of 2 other forks of yaml-rust into a single effort:

@Enselic
Copy link
Collaborator

Enselic commented Apr 17, 2024

Yes that also helps.

It is entirely possible that the ecosystem will reach consensus to use the new fork. I just don't think there is a rush to replace. The most low-risk seems to keep things the way they are.

If any other maintainer want to go ahead with this PR I have no objections. It's just that I don't want to put time on doing it myself. Especially not put time into dealing with the potential problems that could arise.

@Enselic
Copy link
Collaborator

Enselic commented May 30, 2024

There are now at least two other requests to bump this, so the new fork seems to have good momentum. So I'd like to merge this change. I noticed that you deleted the branch, so I can't reopen this, but the diff in #544 looks similar, so we can use that one instead.

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