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 in uucore for illumos and solaris #5489

Merged
merged 4 commits into from
Nov 4, 2023

Conversation

bbarker
Copy link
Contributor

@bbarker bbarker commented Nov 2, 2023

The build was broken on these systems; confirmed working now on illumos. It should also work on solaris after conferring with a solaris developer.

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Cool! The only thing I don't really like is the macro for concatenating arrays. It's unfortunate that the lengths of the array are explicit for example. I'd like to propose this as an implementation for the ALL_SIGNALS static (simplified to just two signals):

#[cfg(target_os = "solaris")]
const SIGNALS_SIZE: usize = 1;

#[cfg(target_os = "illumos")]
const SIGNALS_SIZE: usize = 2;

#[cfg(or(target_os = "solaris", target_os = "illumos"))]
static ALL_SIGNALS: [&str; SIGNALS_SIZE] = [
    "HUP",
    #[cfg(target_os = "illumos")]
    "LWP",
];

@bbarker
Copy link
Contributor Author

bbarker commented Nov 3, 2023

Thanks, that's a definite improvement! Pushed up the change 🙏

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

would it be possible to have a CI for these platforms ?

@bbarker
Copy link
Contributor Author

bbarker commented Nov 3, 2023 via email

@bbarker
Copy link
Contributor Author

bbarker commented Nov 4, 2023

Is it OK to merge without the illumos CI support for now?

I will try to look into it. Have you had good success with your approach to FreeBSD CI?

@Toasterson
Copy link

would it be possible to have a CI for these platforms ?

I am a Maintainer with the OpenIndiana Project. The illumos reference distribution.
What platform do you need the CI on? We usually have either Jenkins or Buildomat as our CI runners but also have github action templates for VM's Seel Chezmoi for a contributed example https://github.com/twpayne/chezmoi/blob/v2.9.3/.github/workflows/main.yml#L173-L191

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Nov 4, 2023

Thanks for your input! We usually use GitHub Actions. If you could point us towards good templates or help us out with creating one, that would be great!

Also I don't think this PR is blocked by not having CI. Adding CI is a task that is big enough to be a separate PR as it will probably uncover more problems to fix.

@tertsdiepraam tertsdiepraam mentioned this pull request Nov 4, 2023
@tertsdiepraam tertsdiepraam merged commit 44d105d into uutils:main Nov 4, 2023
48 of 51 checks passed
@tertsdiepraam
Copy link
Member

Thanks! I've made an issue for the CI here: #5492. @bbarker feel free to pick that up if you want!

zhitkoff pushed a commit to zhitkoff/coreutils that referenced this pull request Nov 6, 2023
* uucore support for illumos and solaris

* use macro to consolidate illumos and solaris signals

* fixing some CI issues

* replaced macro with better cfg usage
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.

4 participants