-
Notifications
You must be signed in to change notification settings - Fork 849
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
PLT-7540: Automate build process + code-signing for Windows/Mac #676
Conversation
…application in a Docker container
…d permissions issues
…ackaging succeeds
…eating directories in root. Wine wants to make stuff too, and keeping track of all the build folders is brittle
@MusikPolice Is there a rough ETA on when this PR might be ready for review? Really excited for this PR :) |
@jasonblais There are three things left to do:
As for timeline, if we just do step 1, this is ready to be deployed, and we can use it to replace Circle CI with Jenkins. Steps 2 and 3 are an enhancement that remove the existing manual code signing step that we have to do with every release. |
…packaging runs on a single executor
build/Jenkinsfile
Outdated
steps { | ||
echo 'Building Mattermost Desktop App' | ||
sh 'rm -rf release' | ||
sh 'npm install' |
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.
Please use yarn
to respect yarn.lock
.
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.
Updated command to use yarn install, @yuya-oc can you give this another review?
build/Jenkinsfile
Outdated
echo 'Building Mattermost Desktop App' | ||
sh 'rm -rf release' | ||
sh 'rm -rf codesign-*' | ||
sh 'npm install' |
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.
Please use yarn
to respect yarn.lock
.
build/Jenkinsfile
Outdated
|
||
echo 'Building Mattermost Desktop App' | ||
sh 'rm -rf release' | ||
sh 'npm install' |
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.
Please use yarn
to respect yarn.lock
.
build/Jenkinsfile
Outdated
sh 'mv release/mattermost-desktop-*.tar.gz release/mattermost-desktop-linux-x64.tar.gz' | ||
sh 'mv release/tmp-linux-ia32.tar.gz release/mattermost-desktop-linux-ia32.tar.gz' | ||
sh 'mv release/mattermost-desktop_*_i386.deb release/mattermost-desktop-linux-i386.deb' | ||
sh 'mv release/mattermost-desktop_*_amd64.deb release/mattermost-desktop-linux-amd64.deb' |
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 would be able to use template feature of electron-builder to rename artifacts.
https://www.electron.build/configuration/configuration#artifact-file-name-template
#582 has some examples of template.
…n numbers in artifacts
@MusikPolice Is the plan to now only code sign the release branches, similar to what we do for the server? |
@jasonblais I think so. I'll have to look into how to actually make that secure though. I don't want to just blindly sign any branch that as a "release" prefix on it's name, because any developer can create one. We may need to add an optional boolean parameter that controls whether or not the job will sign the builds. This parameter would default to |
@MusikPolice Yeah, that would make sense. It might also be worthwhile to check how the code signing is done for the server. |
@MusikPolice Just curious, and not required for now. https://www.electron.build/code-signing Are you considering to use it in future? I think that the feature would make scripts simple. |
@AndersonWebStudio I believe you're taking over this PR now. If you have any questions, don't hesitate to let @yuya-oc or Joram know. |
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.
Almost looks good to me. Now v4.1 uses different artifact name, so please check my comments.
sh '''#!/bin/bash | ||
WIN_IA32_SETUP_VERSION=$(awk -F' ' '{print $3}' <<< $(awk -F'-' '{print $2}' <<< $(ls release/win-ia32/Mattermost\\ Setup\\ *-ia32.exe))) | ||
echo "WIN_IA32_SETUP_VERSION is $WIN_IA32_SETUP_VERSION" | ||
mv release/win-ia32/Mattermost\\ Setup\\ $WIN_IA32_SETUP_VERSION-ia32.exe release/mattermost-setup-$WIN_IA32_SETUP_VERSION-win32.exe |
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.
release/win-ia32
-> release/squirrel-windows-ia32
TEMP_VERSION=$(awk -F' ' '{print $3}' <<< $(ls release/win/Mattermost\\ Setup\\ *[^ia32].exe)) | ||
WIN_64_SETUP_VERSION=${TEMP_VERSION%%.exe} | ||
echo "WIN_64_SETUP_VERSION is $WIN_64_SETUP_VERSION" | ||
mv release/win/Mattermost\\ Setup\\ $WIN_64_SETUP_VERSION.exe release/mattermost-setup-$WIN_64_SETUP_VERSION-win64.exe |
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.
release/win
-> release/squirrel-windows
echo 'Packaging for MacOS' | ||
sh '''#!/bin/bash | ||
npm run package:mac | ||
MACOS_VERSION=$(awk -F'-' '{print $2 }' <<< $(ls release/Mattermost-*-mac.tar.gz)) |
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.
tar.gz
-> zip
npm run package:mac | ||
MACOS_VERSION=$(awk -F'-' '{print $2 }' <<< $(ls release/Mattermost-*-mac.tar.gz)) | ||
echo "MACOS_VERSION is $MACOS_VERSION" | ||
mv release/Mattermost-*-mac.tar.gz release/mattermost-desktop-$MACOS_VERSION-macos.tar.gz |
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.
tar.gz
-> zip
echo "MACOS_VERSION is $MACOS_VERSION" | ||
mv release/Mattermost-*-mac.tar.gz release/mattermost-desktop-$MACOS_VERSION-macos.tar.gz | ||
''' | ||
archiveArtifacts artifacts: 'release/mattermost-desktop-*-macos.tar.gz' |
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.
tar.gz
-> zip
@@ -0,0 +1,138 @@ | |||
#!/usr/bin/env groovy |
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.
Just curious, is shebang needed? In the Jenkinsfile doc, shebang is not used.
} | ||
} | ||
stage('Build Windows') { | ||
agent any |
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.
Just curious, can we use Windows slave in future? We would be able to use Node's native modules.
agent any | ||
steps { | ||
echo 'Building Mattermost Desktop App' | ||
sh '''#!/bin/bash |
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 that set -e
is needed for each bash script in order to stop the pipeline as script error when an error occurred in a certain command.
@AndersonWebStudio After we merge this, how do we cut new release? Is it enough just pushing a tag? |
We have moved this pipeline to the shared-pipelines repo, this pull request no longer needs to be merged. |
Before submitting, please confirm you've
Summary
A Jenkins Pipeline that is capable of building and code signing the desktop application for Windows, MacOS, and Linux. The corresponding Jenkins job is here.
Once merged, the job configuration will be updated so that this runs on any change to any branch in the desktop repository. That way, the existing mattermost-desktop-cut-release can still be used to cut releases. It creates a new branch, which this job will pick up and build.
We may want to think about artifact naming before we start using this to build releases. Currently, this pipeline removes version numbers from all artifacts that it builds, but we might not want to do that, since it could lead to confusion when trying to download the latest artifacts for release testing.
Finally, sorry for all of the commits to this branch. There isn't a great way to test this stuff other than to commit it and run it on Jenkins, since setting up my own Jenkins instance with EC2 spot instances and AMIs and such is a pain. Just look at the final script. The interim commits don't matter, and they'll all be squashed down to one when we merge anyway.
Issue link
https://mattermost.atlassian.net/browse/PLT-7540