Closed EliRibble closed 4 years ago
gerickson
commented
4 years ago Can you comment on what failed with the prior regular expression and what the new regular expression addresses?
It looks like you're adding support for extended regular expressions, for portability, you should probably use -E vs. -r.
gerickson
commented
4 years ago I created the attached script protoc-version-test.sh.zip, based on running git tag -l based on a clone of https://github.com/protocolbuffers/protobuf.git. The script attempts to use both the baseline regular expression, both using BSD and GNU sed, as well as the proposed regular expression, both using BSD and GNU sed.
At least for my environment, the proposed regular expressions fail with:
/usr/local/gnu/bin/sed: -e expression #1, char 1: unknown command: `-'for GNU sed and:
sed: 1: "-r
": invalid command code -for BSD sed. For the baseline regular expression, both BSD and GNU sed successfully handle any x.y.z version but fail those with "-extra" bits or those of the form x.y.z.p.
EliRibble
commented
4 years ago I haven't investigated this nearly as much as you have at this point. But here's what I have.
Can you comment on what failed with the prior regular expression and what the new regular expression addresses?
Probably not. I did nothing to debug the previous pattern, just wrote my own based on my extremely limited understanding of sed. I didn't even go into this knowing what versions you were targeting.
On my workstation I have the following:
$ protoc --version
libprotoc 3.6.1
$ sed --version
sed (GNU sed) 4.7
Packaged by DebianAnd with those versions the pattern as currently written does not detect my version of protoc.
I'm in the process of getting access to our build servers to determine what their output is. What I do have is this:
configure: error: protobuf version between 3.3.0 and 3.9.1, inclusive, is required. However, /usr/local/google/home/mosaic-role/slave/repo_clients/internal/master/out/host/linux-x86/bin/protoc is version unknown.
Please try downloading and installing 'https://github.com/protocolbuffers/protobuf/releases/download/v3.3.0/protoc-3.3.0-linux-x86_64.zip' or later.
make[1]: *** [Makefile:534: config.status] Error 1But I don't think that helps much until you have the specific output of protoc --version and sed --version
EliRibble
commented
4 years ago Our build systems have this sed:
$ sed --version
sed (GNU sed) 4.7
Packaged by Debian
Copyright (C) 2018 Free Software Foundation, Inc.Our build systems have this
sed:$ sed --version sed (GNU sed) 4.7 Packaged by Debian Copyright (C) 2018 Free Software Foundation, Inc.
That's the same version in my attached test for "GNUSED", so at least we have an apples-to-apples comparison there.
EliRibble
commented
4 years ago Okay, from our build servers:
~$ protoc --version
libprotoc 3.7.1It sounds like what I need to do is to update this pull request so that it passes all of the tests in protoc-version-test.sh.zip as well as parsing the version strings from our systems. Is that all that is required to get this merged?
gerickson
commented
4 years ago It sounds like what I need to do is to update this pull request so that it passes all of the tests in protoc-version-test.sh.zip as well as parsing the version strings from our systems. Is that all that is required to get this merged?
Ideally, we can figure out why the test is failing on the build machine since the test file I uploaded indicates that it should work for the version cited. Can we capture the od -x output of protoc --version on the build machine? Maybe there is an escape character or something in there fouling up the regular expression.
EliRibble
commented
4 years ago I'm not going to be able to capture it on the build machine because that takes time and requires access I don't have. I can capture it on my workstation, however, where the sed pattern also fails.
> protoc --version | od -x
0000000 696c 7062 6f72 6f74 2063 2e33 2e36 0a31
0000020I updated my proposed pattern in your script. Looks like this:
${BSDSED} -n -r -e 's/^[^0-9]*(([0-9]+\.)*[0-9]+).*/\1/p')
${GNUSED} -n -r -e 's/^[^0-9]*(([0-9]+\.)*[0-9]+).*/\1/p')The output of your test script is now:
version BSD SED Prior GNU SED Prior BSD SED Proposed GNU SED Proposed
2.4.1 2.4.1 2.4.1 2.4.1 2.4.1
2.5.0 2.5.0 2.5.0 2.5.0 2.5.0
2.6.0 2.6.0 2.6.0 2.6.0 2.6.0
2.6.1 2.6.1 2.6.1 2.6.1 2.6.1
2.6.1rc1 2.6.1 2.6.1
3.0.0 3.0.0 3.0.0 3.0.0 3.0.0
3.0.0-alpha-1 3.0.0 3.0.0
3.0.0-alpha-2 3.0.0 3.0.0
3.0.0-alpha-3 3.0.0 3.0.0
3.0.0-alpha-3.1 3.0.0 3.0.0
3.0.0-alpha-4 3.0.0 3.0.0
3.0.0-alpha-4.1 3.0.0 3.0.0
3.0.0-beta-1 3.0.0 3.0.0
3.0.0-beta-1-bzl-fix 3.0.0 3.0.0
3.0.0-beta-1.1 3.0.0 3.0.0
3.0.0-beta-2 3.0.0 3.0.0
3.0.0-beta-3 3.0.0 3.0.0
3.0.0-beta-3-pre-1 3.0.0 3.0.0
3.0.0-beta-3.1 3.0.0 3.0.0
3.0.0-beta-3.2 3.0.0 3.0.0
3.0.0-beta-3.3 3.0.0 3.0.0
3.0.0-beta-4 3.0.0 3.0.0
3.0.0-javalite 3.0.0 3.0.0
3.0.1-javalite 3.0.1 3.0.1
3.0.2 3.0.2 3.0.2 3.0.2 3.0.2
3.1.0 3.1.0 3.1.0 3.1.0 3.1.0
3.1.0-alpha-1 3.1.0 3.1.0
3.2.0 3.2.0 3.2.0 3.2.0 3.2.0
3.2.0-alpha-1 3.2.0 3.2.0
3.2.0-rc.1 3.2.0 3.2.0
3.2.0rc2 3.2.0 3.2.0
3.2.1 3.2.1 3.2.1 3.2.1 3.2.1
3.3.0 3.3.0 3.3.0 3.3.0 3.3.0
3.3.0rc1 3.3.0 3.3.0
3.3.1 3.3.1 3.3.1 3.3.1 3.3.1
3.3.2 3.3.2 3.3.2 3.3.2 3.3.2
3.4.0 3.4.0 3.4.0 3.4.0 3.4.0
3.4.0rc1 3.4.0 3.4.0
3.4.0rc2 3.4.0 3.4.0
3.4.0rc3 3.4.0 3.4.0
3.4.1 3.4.1 3.4.1 3.4.1 3.4.1
3.5.0 3.5.0 3.5.0 3.5.0 3.5.0
3.5.0.1 5.0.1 5.0.1 3.5.0.1 3.5.0.1
3.5.1 3.5.1 3.5.1 3.5.1 3.5.1
3.5.1.1 5.1.1 5.1.1 3.5.1.1 3.5.1.1
3.5.2 3.5.2 3.5.2 3.5.2 3.5.2
3.6.0 3.6.0 3.6.0 3.6.0 3.6.0
3.6.0.1 6.0.1 6.0.1 3.6.0.1 3.6.0.1
3.6.0rc1 3.6.0 3.6.0
3.6.0rc2 3.6.0 3.6.0
3.6.1 3.6.1 3.6.1 3.6.1 3.6.1
3.6.1.1 6.1.1 6.1.1 3.6.1.1 3.6.1.1
3.6.1.2 6.1.2 6.1.2 3.6.1.2 3.6.1.2
3.6.1.3 6.1.3 6.1.3 3.6.1.3 3.6.1.3
3.7.0 3.7.0 3.7.0 3.7.0 3.7.0
3.7.0-rc.2 3.7.0 3.7.0
3.7.0-rc.3 3.7.0 3.7.0
3.7.0rc1 3.7.0 3.7.0
3.7.0rc2 3.7.0 3.7.0
3.7.1 3.7.1 3.7.1 3.7.1 3.7.1
3.8.0 3.8.0 3.8.0 3.8.0 3.8.0
3.8.0-rc1 3.8.0 3.8.0
3.9.0 3.9.0 3.9.0 3.9.0 3.9.0
3.9.0-rc1 3.9.0 3.9.0
3.9.1 3.9.1 3.9.1 3.9.1 3.9.1
3.9.2 3.9.2 3.9.2 3.9.2 3.9.2
3.10.0 3.10.0 3.10.0 3.10.0 3.10.0
3.10.0-rc1 3.10.0 3.10.0
3.10.1 3.10.1 3.10.1 3.10.1 3.10.1
3.11.0 3.11.0 3.11.0 3.11.0 3.11.0
3.11.0-rc1 3.11.0 3.11.0
3.11.0-rc2 3.11.0 3.11.0
3.11.1 3.11.1 3.11.1 3.11.1 3.11.1
3.11.2 3.11.2 3.11.2 3.11.2 3.11.2
3.11.3 3.11.3 3.11.3 3.11.3 3.11.3
3.11.4 3.11.4 3.11.4 3.11.4 3.11.4 sorry, previous chart is incorrect, I accidentally ran two versions of GNU sed, not a BSD sed and a GNU sed
EliRibble
commented
4 years ago I've updated the commit. I don't have an easy way to test BSD sed, I'm not familiar with BSD and it looks like compiling just sed is non-trivial. From running your tests on my system with GNU sed this new pattern seems strictly better in various ways including recognizing patterns like 3.11.0-rc2 as 3.11.0 and not mis-recognizing 3.6.1.3 as 6.1.3.
EliRibble
commented
4 years ago Great, confirmed, this builds on my workstation within our larger system.
Bug: https://github.com/openweave/openweave-wdlc/issues/38