-
Notifications
You must be signed in to change notification settings - Fork 892
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
Snappy: Add support for snapcraft builds #1898
Conversation
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.
Honestly, I may know less about snapcraft than you. I would prefer to
make ci/snapcraft.sh
a bash script with .bash
extension instead to
make use of bash features - array, also because this script is not intended
to used by other parties (just in our CI).
ci/snapcraft.sh
Outdated
# as rustup.$PROXY. If we have an alias which is not a supported proxy name | ||
# then rustup might get sad. | ||
|
||
PROXIES="cargo cargo-clippy cargo-fmt cargo-miri clippy-driver rls rustc rustdoc rustfmt" |
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.
We may want to use bash feature - array instead.
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'm not a shell person. I'm happy if you can tell me what to do there, otherwise this can stay until you want to fix it later
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.
Basic uses of bash array if you want to try: https://devhints.io/bash#arrays
805fb5a
to
0589416
Compare
@lzutao Thank you for the comprehensive review. I believe I've tackled everything you hilighted (except the one thing I don't feel confident in) -- could you recheck please? |
ci/snapcraft.sh
Outdated
# as rustup.$PROXY. If we have an alias which is not a supported proxy name | ||
# then rustup might get sad. | ||
|
||
PROXIES="cargo cargo-clippy cargo-fmt cargo-miri clippy-driver rls rustc rustdoc rustfmt" |
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.
Basic uses of bash array if you want to try: https://devhints.io/bash#arrays
0589416
to
593b35c
Compare
r=me with or with out using bash array, unless others disagree. |
Thank you @lzutao We won't be merging this until I've spoken to the release team about snaps, but it's good to know you're now satisfied with my awful shell code :-D |
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.
Sure, why not.
☔ The latest upstream changes (presumably #1916) made this pull request unmergeable. Please resolve the merge conflicts. |
Now that we're a bit further on in our releases, and Windows builds are no longer always failing; I'll give this some more thought. |
I've started a discussion with Snap people about github actions for this kind of thing. Depending on how that goes, I'll either rework this PR, or close it and open a fresh one for continued discussion around |
593b35c
to
37a0191
Compare
e20c125
to
96a35fd
Compare
I now need to wait for an appropriate publish action to be written and we can proceed further. |
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've left a few drive-by comments after the discussion on the Snapcraft forum. I've been working on an action to publish snaps here:
https://github.com/jhenstridge/snapcraft-publish-action
I haven't quite locked down the API yet, but it is functional. As the action depends on a secret (the Snap Store credentials), I'm not sure how easy it will be to test within a pull request.
96a35fd
to
d6891d2
Compare
@jhenstridge Thank you for your input and review -- I'll look at the publish action you've provided and I can probably test it by adding the secret to my own repo fork and trying it there to ensure it behaves OK. I'll try and do that soon. |
d6891d2
to
5c17fc9
Compare
@jhenstridge I think that ought to do the trick, obviously I'm tied to a particular commit of your repo for now. I tested it over at my own fork and it seemed to do the right thing, so now I need to wait for you to finalise a v1 of the publish action I guess, and then decide on the final comments you made on the snapcraft yaml file. |
655362c
to
d2f3f1f
Compare
I believe everything is right now, and I've added the token to the repo's secrets ready for post-merge use. If noone objects, I'll merge this soon and we can see how that goes. |
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.
One question, one requested change
d2f3f1f
to
de0df60
Compare
Is the shell script shellchecked automatically? |
@rbtcollins Yes, shellcheck is run over all our shell scripts as part of the CI. |
de0df60
to
c64074d
Compare
To begin the work of supporting snaps of rustup, we need to first ensure we can build the snaps in the first place. Signed-off-by: Daniel Silverstone <[email protected]>
Signed-off-by: Daniel Silverstone <[email protected]>
c64074d
to
de2b7a0
Compare
@rbtcollins I believe your question and change were resolved. I've updated to use the now official snapcore actions. I think we're likely good to merge assuming the CI remains green. Opinions? |
I have decided to go ahead and merge this because we need to work through any problems long before we make a release, and we can only do that once it's trying to push to snapstore etc. |
This commit enables building snaps for the various platforms supported by snapcraft.io
Currently the snap is under my personal page, and despite building it here, we'll keep that until after all-hands where I'll have a chat with t-release people about what we might do for an official snap. So far I've had no joy looking at official methods of distributing rustup in other channels such as flatpak.