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

remove utf8 dependency. #4460

Merged
merged 7 commits into from
Apr 14, 2023
Merged

Conversation

publicmatt
Copy link
Contributor

First time contributor. I wanted to help so I plucked an issue with the "good first issue" tag.

This commit addresses #4289

Summary:

The author of one of the wc dependencies opened an issue stating that the crate and repo were being deprecated. They suggested incorporating what was needed into coreutils directly and removing the dependency. I incorporated and built uu_wc which should fix this issue.

Of note: there are a few 'unused' warnings in the incorporated code - should I try to remove that as well?

src/uu/wc/src/utf8/lib.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Contributor

could you please also fix the clippy warnings?
thanks
(in a different commit - ie don't force push)

@tertsdiepraam
Copy link
Member

@sylvestre what's your opinion on the unused code? I think we should delete it if we're not using it.

@sylvestre
Copy link
Contributor

as we are forking the lib, yes

@SimonSapin
Copy link

FWIW this is what I meant by "relevant code" in #4289: copy the parts you actually use.

@tertsdiepraam
Copy link
Member

Indeed! @publicmatt to answer this question then:

Of note: there are a few 'unused' warnings in the incorporated code - should I try to remove that as well?

Let's only use the relevant code like @SimonSapin suggests :)

@publicmatt
Copy link
Contributor Author

That makes sense - thanks for the guidance.

@sylvestre
Copy link
Contributor

@publicmatt do you have an update?

@publicmatt
Copy link
Contributor Author

@publicmatt do you have an update?

I just pushed an update where I removed all dead code from the utf8 crate that was incorporated. I'm not sure what the merge process looks like - should I squash all those commits or does that happen with the pull request?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Mar 27, 2023

should I squash all those commits or does that happen with the pull request?

We usually keep the commits, but we squash sometimes if a contributor made a mess of the history. So I'd be nice if you could clean those up. In this case I think 1 commit would suffice.

@publicmatt
Copy link
Contributor Author

should I squash all those commits or does that happen with the pull request?

We usually keep the commits, but we squash sometimes if a contributor made a mess of the history. So I'd be nice if you could clean those up. In this case I think 1 commit would suffice.

Makes sense! I did the following to combine my mess of commits:

git reset --soft [last commit from main before I started working on this]
git commit -m "replace utf8 dependency"
git push --force

I think that's right??

@tertsdiepraam
Copy link
Member

I usually use interactive rebase for this. But I think it's correct :)

@publicmatt
Copy link
Contributor Author

Getting acquainted with all the checks - I'll fix them up and push again.

@uutils uutils deleted a comment from github-actions bot Mar 28, 2023
@uutils uutils deleted a comment from github-actions bot Mar 28, 2023
@sylvestre
Copy link
Contributor

Needs rustfmt & clippy fixes please :)

@publicmatt
Copy link
Contributor Author

I ran and fixed suggestions from: cargo fmt and cargo clippy and RUSTDOCFLAGS="-Dwarnings" cargo doc --features "feat_os_unix" --no-deps --workspace --document-private-items. These were all failing the checks last time.

src/uu/wc/src/utf8/mod.rs Outdated Show resolved Hide resolved
sylvestre and others added 4 commits April 5, 2023 10:54
Co-authored-by: Daniel Hofstetter <[email protected]>
Co-authored-by: Daniel Hofstetter <[email protected]>
Co-authored-by: Daniel Hofstetter <[email protected]>
@github-actions
Copy link

github-actions bot commented Apr 5, 2023

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

src/uu/wc/src/utf8/mod.rs Outdated Show resolved Hide resolved
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.

5 participants