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

Warn on stale build due to kernel changed #485

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Oct 26, 2023

Description

Closes #207.

kerl Outdated Show resolved Hide resolved
We thus bind creating and finding `uname -r` (label) by means
of a function, and then actually use that function to run the
command, via eval
@@ -985,6 +990,14 @@ fail_do_build() {
fi
}

uname_r_label() {
echo "uname -r"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure this is the right command on all of our platforms?? probably is - but for md5 and sed we had to detect platform before we used a command like this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can check further. At the same time, I'm not sure:

  1. we're always using \ (sometimes we are) where commands are executed
    1.1. I can later pull request for this, maybe (?)
  2. we're using the sed options consistently (don't seem to be)
    2.1. I can later pull request for this, maybe (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uname should be POSIX, and -r is defined as "Write the current release level of the operating system implementation."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I could gather, uname -r is valid for Darwin, OpenBSD, FreeBSD, and a host of Linux systems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. we're always using \ (sometimes we are) where commands are executed

I don't remember the exact commit where it was introduced (think it was some kind of shellcheck clean up???) but it is supposed to make it so the command is executed in its own environment without environment variables or something?

1.1. I can later pull request for this, maybe (?)

Yeah maybe? low priority tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember the exact commit where it was introduced

ShellCheck does not complain about this sort of stuff, AFAIK. If I remember correctly is was an actual bug somebody wanted to fix...

Yeah maybe? low priority tho

I have no real priorities (I'm kinda going, with kerl, with oldest issues first - if that's any type of priority order), but these should mostly be mechanical changes 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ref: #363

kerl Show resolved Hide resolved
kerl Show resolved Hide resolved
@paulo-ferraz-oliveira
Copy link
Contributor Author

I'm squash+merging... Thanks for the review.

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit bfc7521 into kerl:master Nov 1, 2023
9 checks passed
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/message-on-stale-build branch November 1, 2023 21:18
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.

Kerl fails to install previous builds if kernel upgraded
2 participants