Allow configuring the diff command #227
Conversation
|
Hmm, failures in MSYS2 are due to an update of the gcc version used, which in turn incremented the default std version. This is related to https://sourceforge.net/p/gnucobol/discussion/help/thread/2a4601183a. Though: maybe we should add CI checks for different std values (at least for the Ubuntu CI) |
|
Concerning std23: The Gentoo patch looks like what I had in mind - I "only" need to test it on "ancient" compilers (if there are issues that means an ifdef/macro solution) and it likely has to been extended into codegen before, which would be a bit more work - you're welcome to provide another PR for that (for the time being only using a macro if you find that good without conditional compilation). We should have a single CI with the oldest std and checks (I think that's there) and otherwise go with the default one. |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
Changelog entries missing (even goes to tests this time), possibly adjust the actual check within configure.
ddeclerck
force-pushed
the
configurable_diff
branch
5 times, most recently
from
b685cb1 to
6469b87
Compare
|
Added the Changelog entries and updated the config test. |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
Thank you for your work.
Some minor changes are needed, can likely go upstream today.
.github/workflows/windows-msys2.yml
Outdated
| mkdir _build | ||
| cd _build | ||
| ../configure $CFGOPT | ||
| ../configure $CFGOPT CFLAGS=--std=c17 DIFF="/c/Program Files/Git/usr/bin/diff.exe" DIFF_FLAGS="-w" |
There was a problem hiding this comment.
Please clarify: with the adjusted test the setting of DIFF is not necessary any more, is it?
Also: why do we need -w - --strip-trailing-cr is set automatically and should be enough, no?
There was a problem hiding this comment.
We still need to use a "custom diff" (the one from Git-bash) to workaround the diff problems we have in MSYS2 (cf #225 (comment)).
As for -w, last time I checked, it was needed despite using --strip-trailing-cr. I can check again...
There was a problem hiding this comment.
And as you can see by the last run, comparison fails in NIST even if the files are "identical" (and despite using --strip-trailing-cr) ; I don't get what could cause this, but -w seems to work around this...
There was a problem hiding this comment.
Oh, in fact the --strip-trailing-cr flag was not passed on MSYS2 - due to the path to diff containing spaces (was messing with the option detection). Fixed by quoting $DIFF in configure.ac.
There was a problem hiding this comment.
We still need to use a "custom diff" (the one from Git-bash) to workaround the diff problems we have in MSYS2 (cf #225 (comment)).
The point is that the test in configure.ac should go through all available diff binaries in PATH and then set DIFF to the one that does not fail the test, which should be the one from git (in the case that this is not in PATH, please use PATH="$PATH:/c/Program Files/Git/usr/bin" ../configure $CFGOPT CFLAGS=--std=c17 to ensure that the check can find it).
There was a problem hiding this comment.
Ah, yeah, I had forgotten about that part. I'll update accordingly.
ddeclerck
force-pushed
the
configurable_diff
branch
5 times, most recently
from
f4295ea to
2931616
Compare
ddeclerck
force-pushed
the
configurable_diff
branch
from
2931616 to
58d4b7d
Compare
|
I think you have the encoding right now, as an alternative |
GitMensch
left a comment
GitMensch
left a comment
There was a problem hiding this comment.
Good for upstream now - and good that the test is now also verified in the CI :-)
|
Merged in SVN @ 5508. |
2 participants
This PR brings non CI-related changes from PR 225 to GC3.
Essentially, it allows to customize the diff command to use.