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

add support for Illumos #48

Merged
merged 1 commit into from
Jun 13, 2023
Merged

add support for Illumos #48

merged 1 commit into from
Jun 13, 2023

Conversation

siepkes
Copy link
Contributor

@siepkes siepkes commented Jun 12, 2023

This commit introduces support for Illumos, an (Open) Solaris derivative.

uname -o on Illumos returns illumos.

It also clarifies the documentation a bit on why the domainname field is missing for OS'es like the BSD's and Illumos.

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #48 (343298f) into main (6a66620) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   94.97%   94.98%   +0.01%     
==========================================
  Files           4        4              
  Lines         956      958       +2     
==========================================
+ Hits          908      910       +2     
  Misses         48       48              
Flag Coverage Δ
macos_latest 99.33% <100.00%> (+<0.01%) ⬆️
ubuntu_latest 99.33% <100.00%> (+<0.01%) ⬆️
windows_latest 94.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/platform/unix.rs 99.03% <100.00%> (+0.01%) ⬆️

@siepkes
Copy link
Contributor Author

siepkes commented Jun 12, 2023

It seems Github actions dies on a style check in an integration test. Oddly enough this PR doesn't change that code. I think the style check might have been updated? Causing it to suddenly fail on this piece of existing code?

error: redundant clone
  --> tests/integration_test.rs:51:25
   |
51 |     let info_copy = info.clone();
   |                         ^^^^^^^^ help: remove this
   |
note: cloned value is neither consumed nor mutated
  --> tests/integration_test.rs:51:21
   |
51 |     let info_copy = info.clone();
   |                     ^^^^^^^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone
   = note: `-D clippy::redundant-clone` implied by `-D warnings`

error: could not compile `platform-info` (test "integration_test") due to previous error
warning: build failed, waiting for other jobs to finish...
error: redundant clone

@rivy
Copy link
Member

rivy commented Jun 13, 2023

Hmm, odd that it didn't show up as failures in the prior merge with the CI code change.
I'll fix it and you can rebase onto the new 'main'.

Copy link
Member

@rivy rivy left a comment

Choose a reason for hiding this comment

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

Minor nit... we generally use active/imperative voice for commit messages.
When you rebase onto the new 'main', would you mind changing the commit message to "add support for Illumos"?

@rivy rivy changed the title Added support for Illumos add support for Illumos Jun 13, 2023
@rivy
Copy link
Member

rivy commented Jun 13, 2023

@siepkes , the updated 'main' is now available. If you rebase, I expect that all tests should pass.

This commit introduces support for Illumos, an (Open) Solaris
derivative.

It also clarifies the documentation a bit on why the domainname field is
missing for OS'es like the BSD's and Illumos.
@siepkes
Copy link
Contributor Author

siepkes commented Jun 13, 2023

Your changes seem to have worked. All checks passed!

@rivy rivy merged commit 5599af7 into uutils:main Jun 13, 2023
@kulikjak kulikjak mentioned this pull request Sep 19, 2024
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.

2 participants