auto-upload PR AppImages & auto-post link to issue tracker and/or the github PR
In the code shoogle wrote to incorporate the new AppImage build system, I noticed that after building the AppImages, they are simply discarded here:
https://github.com/musescore/MuseScore/blob/master/build/travis/job2_Ap…
But there is a great lost opportunity here to save other developers time who would currently have to recompile the PR if just want to test out the functionality. (Not to mention the time lost having to context switch with whatever branch is currenlty checked out in your git folder, which also overwrites already-compiled object files of all changed source code files). The greater ease to test PR's would result in more useful & informed comments while the PR is under review.
So my feature request would require:
- setup a new *Repository* in bintray that would be named something like "pull-requests-linux" (so are clearly sepearted from the "nightlies-linux" repository)
- remove the lines in job2_AppImage/bintray.sh that currently are used to prevent the PRs from being uploaded to bintray, and add lines to set $APPNAME to something like "musescore-pull-request-#nnnn", and see $BINTRAY_REPO to whatever name decided in step 1.
- have bintray.sh return a list of urls of AppImage from the build script (if successful) and pass to .travis.yml
- have .travis.yml gather all links to generated AppImages from the ./bintray.sh calls, and finally post these AppImage links to:
- the github PR
- the musescore issue tracker (if commit contains fix #xxxxx)
- the IRC chat
Having the AppImage download links immediately accessible without having to do much work or clicking will be a great investment.
Note: maybe to reduce multiple messages, could have travis wait for all builds to finish, and then just send one link containing {test pass/fail result, link to x86-64 linux AppImage} (maybe no need to post links for i686 or arm, to reduce unnecessary info overload, since most devs are on x86-64).
Comments
This is a great idea, unfortunately it's not quite as simple as that. The problem is with how to keep the repo secure so that random people can't upload malicious code. I had been considering something similar, but I haven't thought of a good solution yet.
If we just removed those lines you linked to, that still wouldn't cause pull requests to be uploaded to Bintray. Uploads require these variables to be set, but these are not stored in the file; they are stored encrypted on Travis. However, they are not available for pull requests, otherwise anyone could just submit a PR with the line "
echo $BINTRAY_API_KEY" and read it out of the build log, and then have unlimited access to the repository. Those lines are there simply to prevent the upload from failing due the the secure variables not being set, which would otherwise cause the AppImage build to be marked as failed even though it actually compiled successfully.One way around the problem would be to hard-code the API key for a dummy Bintray account that was specifically there to upload pull requests. That account couldn't be linked to the MuseScore account though otherwise it could upload to the other MuseScore repos. Also, the API key probably allows the user to do more than just upload to the repo so we'd have to look very carefully at that option.
Another way around it would be to redirect the pull request uploads to a dedicated server that would only accept uploads from Travis. The server would then forward the upload to Bintray, thus keeping the API key secret. Of course, setting up this server would require quite a bit of work, and it would almost certainly not be free, unlike Bintray.
In reply to This is a great idea, by shoogle
I did not consider the security issue about API_KEY.
Everything you're saying makes sense now. I agree with your everything you saying now.
Fortunately now, we at least do have the ability for oridinary users to have travis auto-build every branch and to auto-upload the build to their bintray. Maybe instead of going to effort to investiage a more complicated/expensive solution, I can just edit the musescore git workflow page to include instructions on how to create & confingure both travis & bintray account to use bintray, and maybe send email to dev list. Maybe could we say that setting up their branches to auto-upload be a strong recommendation if they want their PR to be considered, since that makes their PRs more easily testable.
In reply to I did not consider the by ericfontainejazz
I wouldn't go that far, we want to reduce the workload for contributers, not increase it ;)
People shouldn't really test PRs without looking at the code first. I know they do it anyway, but it's not something we should encourage. There is probably a way we could make it easy for lasconic to trigger an AppImage to be build for a particular PR if he thinks lots of people will want to test it.
In reply to I wouldn't go that far, we by shoogle
Good point, we don't want to scare away new devs.
I'll add instructions under the "Tips & Tricks" section of the musescore git workflow page on how to setup travis & bintray. (No metion of any requirement or suggestion...just a tip).
I sometimes compile someone's PR to test it (after looking at the code), especially if it is someone who fixed an bug or feature request that I filed. But compiling takes a long time, so having acces to a precompiled version does save me a lot of time.
In reply to Good point, we don't want to by ericfontainejazz
I would be very interested in the instruction for setting up travis (and possibly bintray) for a personal GitHub account. Thank you Eric.
In reply to I would be very interested in by ABL
I made directions here:
https://musescore.org/en/developers-handbook/git-workflow#Run-tests-on-…
https://musescore.org/en/developers-handbook/git-workflow#Upload-AppIma…
But I haven't tested that with a fresh Travis & Bintray account. If you find any errors, please update the steps. Note that currently you have to list the name of your branch in Travis environment varialbe APPIMAGE_UPLOAD_BRANCHES. (Once https://github.com/musescore/MuseScore/pull/2438 or something like it gets merged, then can use string "ALL" to auto upload commits to all branches)
In reply to Good point, we don't want to by ericfontainejazz
A tip is good, no requirement.
For the PR I merge (I'm not the only one with this right btw, Werner does that too occasionnally and I believe Marc and Miwarre can do this too), I build them on my mac machine btw and depending on my location it can faster than downloading an appimage (that would not run on my mac :) So the AppImage wouldn't have a big impact for me, but I understand how it would help other to have them available and I hope they will use them to comment on the PR!
However, I can't think about any secure way to have this process streamlined. If a user followed the "tip" then he can always link to his bintray account in a PR comment. (this build will be generated from his branch btw, not from the PR, since PR can't have access to any encrypted data at all).
In reply to A tip is good, no by [DELETED] 5
If we wanted to do this at some point in the future it could probably be made secure by creating a separate Bintray account just for PR uploads and then hiding the API key in an obfuscated binary, which would simply set the API_KEY environment variable and then call bintray.sh, having first checked to ensure that bintray.sh hadn't been modified in any way (e.g. by comparing a checksum). Or, even better, the binary would be a replacement for /usr/bin/curl with the API key embedded. In the unlikely event that someone did go to the trouble of finding a way to circumvent this security they would only gain access to the repo belonging to the PR account and not the the proper MuseScore account. But this isn't really a priority so I think letting people build for themselves will do fine for the present.
In reply to If we wanted to do this at by shoogle
Naw, lets not do something insecure...worst case the PR bintray account gets hacked and the uploaded binares get replaced by malicious binaries.
Here's something that might work, but will only post the link if developers setup their personal travis & binray to auto build: after MuseScore's Travis finishes runing tests on a PR, Travis will check if there already exists a binary corresponding to the PR in the developer's bintray account, and if it exists, then travis will post that link to the github PR page. (Note that the test script takes longer than the AppImage compilation, so provided there are no queue reordering, then by the time the travis PR test finishes, then the developer's bintray upload should be complete.)
Edit: developer AppImages need to adhere to a strict naming scheme (and take into account if a PR is revised with push -f)
In reply to Naw, lets not do something by ericfontainejazz
I really don't think anyone would bother hacking it, even if the key was in plain text, mainly because there would be nothing of value and because the binaries would only be run by developers who are less likely to be conned by ransomware, etc, and also because nobody would know it was there to look for it in the first place. However, this feature has actually been requested on Travis, so it's probably worth waiting to see the outcome of that request rather than implementing our own workaround. (You'll notice there are links to similar projects that have implemented their own security - or none!)
Edit: The test build doesn't always finish after the AppImage builds BTW. Sometimes the AppImage builds don't start until 20 minutes or more after the tests, probably because Travis assigns them a lower priority in the queue because they are set to be allowed to fail without failing build as a whole.
In reply to I really don't think anyone by shoogle
good point about the race condition.
I see you made a comment on that Travis issue, so I guess you'll be notified if they fix it, so I guess wait for that :)
Could something like https://transfer.sh be used?
Seems easy enough:
# Uploading is easy using curl
$ curl --upload-file ./hello.txt https://transfer.sh/hello.txt
https://transfer.sh/66nb8/hello.txt
# Download the file
$ curl https://transfer.sh/66nb8/hello.txt -o hello.txt
Files are stored for 14 days which should be plenty for PRs.
However there is absolutely no authentication whatsoever, so we would need to think carefully about the security implications.
https://github.com/projectatomic/bubblewrap could be used to run these AppImages isolated with read-only access to the host filesystem. (Didn't spend much time with it so far so far, so I didn't succeed in running the whole AppImage inside bubblewrap yet but I can mount the AppImage and then run AppRun through bubblewrap successfully).
In reply to Could something like by probono
Looks interesting! An ideal solution would result in an AppImage that works like a normal AppImage but that restricts itself to read-only filesystem access. We'd need a second (secure) server that would repackage the insecure AppImage to enforce read-only access. I'm thinking of something like this:
1. Pull request creates insecure AppImage on Travis and uploads it to Transfer.sh.
2. Pull request informs the secure server that it just made a new AppImage.
3. The secure server downloads the AppImage, extracts it, and repackages it with read-only access.
4. The newly secured AppImage is uploaded to Bintray and a link posted in the PR.
So we would make Bubblewrap part of the AppRun script that would be injected into the insecure AppImage on the secure server.