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

Makefile: rm .SHELLFLAGS, add set -e #105

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Feb 5, 2022

Apparently GHA CI Mac OS X environment uses old GNU Make (v3.81) which
does not yet support .SHELLFLAGS (added by commit 8589e2e).

Since we call a few commands (under for) from the make target,
intermediate failures are ignored unless -e shell option is set (or
explicit && is used, which is not possible to do in for).

Example:

for cmd in false true; do $cmd; done

will succeed unless set -e is set, because the last command executed is true and its exit code is 0.

To fix, use explicit set -e where we execute more than a single command.

Also, replace && with ; for multiple commands. Technically this is not
required for the fix, but since having set -e results in implicit && everywhere,
there is no need to have && explicitly. In other words, this change is
more like a reminder that we do not have to use && here.

Fixes: 8589e2e

Fixes: #99

Apparently GHA CI Mac OS X environment uses old GNU Make (v3.81) which
does not yes support .SHELLFLAGS (added by commit 8589e2e).

Since we call a few commands (under "for") from the make target,
intermediate failures are ignored unless "-e" shell option is set
(or explicit "&&" is used, which is not possible to do in "for").

Example:

	for cmd in false true; do $cmd; done

This code succeeds (has exit code of 0), unless "set -e" is set, because
the last command executed is "true" with the exit code of 0.

Use explicit "set -e" where needed. While at it, also add "-u" to catch
possible uses of uninitialized variables.

Also, replace && with ; for multiple commands. Technically this is not
required for the fix, but since having set -e results in implicit &&,
there is no need to have && explicitly. In other words, this change is
more like a reminder that we do not have to use && here.

Fixes: 8589e2e
Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Collaborator Author

More thoughts on topic:

  1. We have to use cd from Makefile, because go tools (and golangci-lint) treat current directory in a special way, and does not support ways to specify an alternative directory.

  2. Traditionally, such issues are solved with sub-Makefiles, but I don't like it for this repo (since it's easy, and maintaining more Makefiles, even trivial, seems more of a mess than a few fors in the single Makefile). I might reconsider that if we'll have more similar issues.

  3. (Not really related to this fix, but) there are a few alternatives to using subshell to cd back to the top-level directory. They all have their deficiencies, for example:

  • pushd/popd won't work with dash (the default sh on Ubuntu);
  • using cd $pkgdir; test; cd .. would not work in case the hierarchy of packages is not flat (e.g. PACKAGES = mount mountinfo symlink nice/one nice/two); currently this is not the case but being future-proof feels nice;
  • using $PWD (or pwd) in some way (e.g. TOPDIR=$$PWD; ...; cd $TOPDIR) is probably OK, too, but maybe it looks way too verbose.

Using subshell, of course, is not free from deficiencies, too:

  • it's not the best way in terms of performance (but we don't care in this case);
  • the "cd .." is implicit, which I guess hurts readability;
  • it might look cryptic to people not very familiar with shell syntax.

@kolyshkin
Copy link
Collaborator Author

Fedora job failed because of dnf repo glitch, and apparently the retry doesn't work :(

CI restarted.

@thaJeztah
Copy link
Member

Yeah, not sure if I have better options; something like make -C / $(MAKE) -C, but then we need to do trickery as well to still make it read the Makefile from the "parent" directory. Of course we could define targets for each module, and call those, but that would produce a lot of duplication as well.

I guess this change is ok (at least it doesn't make things worse)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

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.

CI: mac os x linter warnings ignored
2 participants