-
Notifications
You must be signed in to change notification settings - Fork 478
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
Sanitize nightly version string which had leading zeroes #563
base: master
Are you sure you want to change the base?
Conversation
Needs to be squashed, but the Github editor doesn't let me do that. |
translate.js
Outdated
@@ -21,6 +24,11 @@ function versionToSemver (version) { | |||
if (version.indexOf('0.3.5-0') !== -1) { | |||
return '0.3.5'; | |||
} | |||
// This parses the obsolete nightly style where the date can have leading zeroes. | |||
var nightly_parsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0([1-9])\.0([1-9])(.*)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work for this one nightly but technically non-zero could also happen on the first position in the day or month.
I have just pushed ethereum/solc-bin#103 and we might actually have some nightlies like that in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm? This regex checks zeroes for months/days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you mean or. Yes, you're correct, need to improve it for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run this on old nightlies? If there is no such instance, I think keeping this simple is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad news. We have several 0.3.6 nightlies that need this:
File name | Reported version |
---|---|
soljson-v0.3.6-nightly.2016.8.27+commit.91d4fa47.js |
0.3.6-nightly.2016.08.27+commit.91d4fa47.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.27+commit.91d4fa47.js |
0.3.6-nightly.2016.08.27+commit.91d4fa47.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.29+commit.b8060c55.js |
0.3.6-nightly.2016.08.29+commit.b8060c55.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.29+commit.b8060c55.js |
0.3.6-nightly.2016.08.29+commit.b8060c55.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.30+commit.cf974fd1.js |
0.3.6-nightly.2016.08.30+commit.cf974fd1.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.30+commit.cf974fd1.js |
0.3.6-nightly.2016.08.30+commit.cf974fd1.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.31+commit.3ccd1986.js |
0.3.6-nightly.2016.08.31+commit.3ccd1986.Emscripten.clang |
soljson-v0.3.6-nightly.2016.8.31+commit.3ccd1986.js |
0.3.6-nightly.2016.08.31+commit.3ccd1986.Emscripten.clang |
I have also detected another variant that triggers a crash:
File name | Reported version |
---|---|
soljson-v0.3.4-nightly.2016.6.6+commit.e97ac4fb.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.6+commit.e97ac4fb.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.093790d7.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.093790d7.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.ccddd6fd.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.ccddd6fd.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.d593166d.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
soljson-v0.3.4-nightly.2016.6.8+commit.d593166d.js |
0.3.4-0/Release-Emscripten/clang/Interpreter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 0.3.4-0
case is partially handled, for 0.1.3-0
and 0.3.5-0
. For both it was understood that the commit hash was set to 0.
However based on your comment it seems that happened for nightlies only? Can you check 0.1.3 and 0.3.4 what they output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameel you may have missed these follow up questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I was sure I answered everything here. Apparently missed your question about releases.
Releases obviously do not have this problem because they do not include a date. You can check that on the list I prepared in the past when I was rebuilding binaries: ethereum/solc-bin#64 (comment)
2c4906f
to
d6c053a
Compare
@cameel updated, how about now? |
6d804b3
to
dba57ba
Compare
translate.js
Outdated
if (version.indexOf('0.3.5-0') !== -1) { | ||
return '0.3.5'; | ||
} | ||
// This parses the obsolete nightly style where the date can have leading zeroes. | ||
const nightlyParsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0?([1-9])\.0?([1-9])(.*)$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ([0-9]+)\.0?([1-9])\.0?([1-9])(.*)
does not look quite right to me. Or am I missing something?
It looks to me like it would not accept two-digit month and day values that don't start with 0
. Like 12
or 27
. The test does seem to parse 2016.08.27
correctly though. Why?
- Is it because it pulls the final
7
into the.*
part? - Will it accept the
12
in0.4.8-nightly.2016.12.16+commit.af8bc1c9.Emscripten.clang
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can fix it by doing this instead:
const nightlyParsed = version.match(/^([0-9]+\.[0-9]+\.[0-9]+)-nightly\.([0-9]+)\.0?([1-9]|1[012])\.0?(3[01]+|[12]?[0-9])(.*)$/);
So the one and two-digits days and months (excluding leading zeroes) are included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@r0qs Do you want to take over reviewing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameel I took a look in this PR, and my suggestion actually does not work for cases like: 2018.2.32
, which would wrongly match: 2018, 2, and 3 instead of 32. Although 32 is not a valid day, the idea here is not to validate the date but only to match broken versions that contain the leading zeros. If a match does not happen, it will just return the version, assuming it is semver compatible.
So I changed @axic regex slightly to support cases like: 2016.12.06
. Thus covering all the leading zero possible combinations for days and months: yyyy.0m.0d, yyyy.mm.0d, yyyy.0m.dd. Any version with a date that does not match the regex will be returned as it is.
dba57ba
to
498cf09
Compare
498cf09
to
2a46464
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me that the regex now correctly match and exclude the leading zeroes.
Needs a rebase though.
2a46464
to
d156528
Compare
Fixes #562.