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

Fix tools/check-parser-uptodate-or-warn.sh #8979

Merged
merged 7 commits into from
Sep 26, 2019

Conversation

gasche
Copy link
Member

@gasche gasche commented Sep 25, 2019

This PR fixes #8965 (a build failure caused by a warning script) by:

  • adding support for Busybox' stat to the script
  • making the script more robust to unexpected failures
  • disabling the script on non-development versions (as suggested by @Octachron)

@gasche gasche changed the title Fix makefile.menhir Fix tools/check-parser-uptodate-or-warn.sh Sep 25, 2019
@dra27
Copy link
Member

dra27 commented Sep 25, 2019

Rather than peppering the diff with suggestions, I've pushed commits any of which you should feel free to blow away - the only thing which has to be fixed is the overly long line (which I fixed by adding an extra error message)

then
exit 0
fi
grep -Fq '+dev' VERSION || exit 0
Copy link
Member Author

Choose a reason for hiding this comment

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

How portable is this change to different grep implementations? I'm worried about using anything more than the simplest grep command.

Copy link
Member

Choose a reason for hiding this comment

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

I already checked - -F and -q are both POSIX, as is the combination of them to -Fq!

Copy link
Member

Choose a reason for hiding this comment

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

(for the others, the -F is overkill, for this one it seems sensible in case some naughty grep treats + as magic without enabling EREs)

@XVilka
Copy link
Contributor

XVilka commented Sep 26, 2019

By the way, I recommend to use shellcheck tool (and it integrates nicely with both Vim and Emacs) for checking the shell scripts on portability issues and such, they even have an online interface.

@XVilka
Copy link
Contributor

XVilka commented Sep 26, 2019

May I ask, when this landed in the trunk, will it be a part of the future 4.09.1 release, or only for 4.10.0 one?

@dra27
Copy link
Member

dra27 commented Sep 26, 2019

There's no reason not to push it to the 4.09 branch, but there's no guarantee that there will be a 4.09.1 release.

@gasche gasche merged commit f075ab6 into ocaml:trunk Sep 26, 2019
@gasche
Copy link
Member Author

gasche commented Sep 26, 2019

(I decided to wait for the INRIA CI reports before backporting into 4.09.)

gasche added a commit that referenced this pull request Sep 26, 2019
Fix tools/check-parser-uptodate-or-warn.sh

(cherry picked from commit f075ab6)
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.09: # stat: can't read file system information for '%m': No such file or directory
3 participants