Skip to content

Conversation

@CorrodedCoder
Copy link
Contributor

Solaris 5.10 uname doesn't support the -o switch, and illumos started with version 5.11 so shortcut the logic to report 'solaris' in such cases where the version is 5.10 or below.

@eli-schwartz
Copy link
Member

eli-schwartz commented Sep 18, 2023

Thanks. The changes look good but the commit message could use a bit of tweaking to describe what is being solved in the first line. "Update ${filename}" isn't very descriptive :( and makes it difficult to locate a commit by viewing the git log or even the web UI at https://github.com/mesonbuild/meson/commits/master

(This is the same problem as the PR title, by the way.)

@CorrodedCoder
Copy link
Contributor Author

Yes, my bad - I'm not particularly used to contributing to GitHub projects other than my own and I was just intending to show my change rather than actually submitting it.
I don't mind if someone else would like to submit the change themselves, or if you'd rather I try to improve the situation I will attempt it, but you may need to baby sit me a little.

@dcbaker dcbaker linked an issue Sep 18, 2023 that may be closed by this pull request
@dcbaker dcbaker added the OS:solaris/illumos Issues specific to the Solaris and illumos OSes label Sep 18, 2023
@eli-schwartz
Copy link
Member

eli-schwartz commented Sep 18, 2023

Command line instructions:

  1. Make sure you have a git clone of your forked PR. git clone it if you have to, then "git checkout" the PR branch.

  2. Then you can simply use git commit --amend, and it will offer you the existing commit message (the most recent commit on that branch, specifically) to edit and fix up.

  3. git push -f to update the PR.

@tristan957 tristan957 added this to the 1.2.2 milestone Sep 18, 2023
@dcbaker
Copy link
Member

dcbaker commented Sep 25, 2023

@CorrodedCoder This looks good from a code point of view. If you could update the commit message, I'd like to merge this for inclusion in the next point release.

The logic previously added to distinguish between illumos and Solaris made use of a uname invocation with a -o switch which is not supported on Solaris 5.10 or earlier.
illumos started with version 5.11 so the logic has been shortcut to report 'solaris' in such cases where the version is 5.10 or below.
@CorrodedCoder
Copy link
Contributor Author

I'm using GitHub Desktop but I think I was able to follow the instructions Eli suggested. Please feel free to let me know if I misunderstood.

@dcbaker
Copy link
Member

dcbaker commented Sep 25, 2023

Looks good to me. Thanks!

@dcbaker dcbaker merged commit 5b16e83 into mesonbuild:master Sep 25, 2023
@eli-schwartz
Copy link
Member

Thank you! I haven't used GitHub Desktop myself (so I can't give instructions for it) but whatever you did there seems to be exactly what I suggested anyway, so that's all good. :)

@eli-schwartz
Copy link
Member

Meh. The github web UI sucks for review.

I looked at this with git log, and for future reference, commit messages should ideally be reflowed to ~80 character long lines, but this was not.

Not quite the end of the world, sure...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OS:solaris/illumos Issues specific to the Solaris and illumos OSes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to build on Solaris 5.10 with meson 1.2.1

4 participants