-
Notifications
You must be signed in to change notification settings - Fork 229
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
Fixup nimbus-fml to download from archive.mozilla.org #5898
Fixup nimbus-fml to download from archive.mozilla.org #5898
Conversation
local patch=${number_string##*.} | ||
if [[ ${#patch} -gt 2 ]] ; then | ||
# rust-component-swift tags have a middle `.0.` to force it to align with spm. We remove it | ||
AS_VERSION=${number_string//\.0\./\.} | ||
else | ||
AS_VERSION=${number_string} | ||
fi |
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.
to clarify the issue, and this is where it's a little tricky, the rust-component-swift version (number_string
) we have here will always be 3 components. It's the AS_VERSION that may be two or three components. I can't think of an easy way to find out whether it's two or three components from here other than sending a network request and seeing if it's not a success
@bendk This makes me think we should think about policy a little, there are a few policy fixes for the future:
- If we make AS always be three or always be two components this will be simpler
- Maybe we could somehow decouple the version of the AS and the version of the FML?
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.
so this code maybe worth commenting.
The SPM has three components; this code checks that last component as the patch
version (${number_string##*.}
, then checks the length of the patch version component (${#patch}
). If it's greater than 2, then treat it like a date stamp, and use it as a nightly. I figured that anything more than121.0.99
would be pathological as a release number.
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.
Aaah I did misinterpret this so thanks for the clarification! Unfortunately, the issue is that the release version could be 121.1
or 121.0.1
, but the scrip only sees 121.0.1
so we may want to strip the middle zero even on releases. You are correct however that nightlies should always be 121.TIMESTAMP
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.
Ok, going to talk in slack, then document here.
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.
Decided to not to look for a forever solution until mozilla-mobile/firefox-ios#17143 lands, then re-evaluate.
This PR should be good for then too.
5e0f7a5
to
c738b9f
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.
LGTM! Thanks for fixing this.
Fix EXP-3977
Pull Request checklist
[ci full]
to the PR title.Branch builds: add
[firefox-android: branch-name]
to the PR title.