Skip to content
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

Unify android build server script with the desktop equivalent #5161

Merged

Conversation

albin-mullvad
Copy link
Collaborator

@albin-mullvad albin-mullvad commented Sep 18, 2023

The scripts deviated some time ago so this commits aims to unify the android script with the desktop equivalent in terms of its structure as well as how it handles versioning and artifacts.

When running this in a local environment, the buildserver-build-android.sh generates the following type of output in the upload directory (updated from the original PR description):

upload
├── 2023.5-beta1-dev-a8bcc8+androiddummy-signed-tag-6+myhost123.sha256
├── 2023.5-beta1-dev-b60c95+androiddummy-signed-tag-7+myhost123.sha256
├── MullvadVPN-2023.6-beta1-dev-a8bcc8+androiddummy-signed-tag-6.aab
├── MullvadVPN-2023.6-beta1-dev-a8bcc8+androiddummy-signed-tag-6.apk
├── MullvadVPN-2023.6-beta1-dev-b60c95+androiddummy-signed-tag-7.aab
└── MullvadVPN-2023.6-beta1-dev-b60c95+androiddummy-signed-tag-7.apk

This change is Reviewable

@linear
Copy link

linear bot commented Sep 18, 2023

DROID-197 Unify android build server script with the desktop equivalent

The upload script that runs on the build server (~ci/buildserver-upload.sh~) needs to be updated to properly handle Android artifacts.

It seems to be rather the build server script that needs to be adjusted.

@albin-mullvad albin-mullvad added the Android Issues related to Android label Sep 18, 2023
@albin-mullvad albin-mullvad self-assigned this Sep 18, 2023
@albin-mullvad albin-mullvad marked this pull request as ready for review September 18, 2023 15:54
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That output doesn't look right to me. Shouldn't it be the following?

upload
├── MullvadVPN-2023.6-beta1-dev-<hash>.aab
├── MullvadVPN-2023.6-beta1-dev-<hash>.apk
└── MullvadVPN-2023.6-beta1-dev-<hash>.sha256

where MullvadVPN-2023.6-beta1-dev-<hash>.sha256 contains the hash for both files? Both files are are available when upload command is called so I don't understand why this happens.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's probably right. I might have missed something. I noticed that the upload script didn't work as expected when running it locally, which is probably related to this.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)


ci/buildserver-build-android.sh line 112 at r1 (raw file):

    fi

    (cd "$artifact_dir" && upload "$version") || return 1

Can't we just do:

cd $artifact_dir
upload "$version"

The script already has set -eu flag on top so any error should cancel execution? This would also go for the other cases of return 1


ci/buildserver-build-android.sh line 114 at r1 (raw file):

    (cd "$artifact_dir" && upload "$version") || return 1
    # shellcheck disable=SC2216
    yes | rm -r "$artifact_dir"

If we are piping in yes we might as well just do: rm -rf "$artifact_dir" directly?

Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)


ci/buildserver-build-android.sh line 112 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Can't we just do:

cd $artifact_dir
upload "$version"

The script already has set -eu flag on top so any error should cancel execution? This would also go for the other cases of return 1

It would have to be

pushd "$artifact_dir"
upload "$version"
popd

But yeah that would be easier to read.

Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Rawa)

Copy link
Collaborator Author

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @raksooo and @Rawa)


ci/buildserver-build-android.sh line 112 at r1 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

It would have to be

pushd "$artifact_dir"
upload "$version"
popd

But yeah that would be easier to read.

@Rawa agreed to keep this as-is since it's the same as the desktop script, right?


ci/buildserver-build-android.sh line 114 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

If we are piping in yes we might as well just do: rm -rf "$artifact_dir" directly?

@Rawa agreed to keep this as-is since it's the same as the desktop script, right?

Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @raksooo)


ci/buildserver-build-android.sh line 114 at r1 (raw file):

Previously, albin-mullvad wrote…

@Rawa agreed to keep this as-is since it's the same as the desktop script, right?

Correct! 👍

Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


ci/buildserver-build-android.sh line 112 at r1 (raw file):

Previously, albin-mullvad wrote…

@Rawa agreed to keep this as-is since it's the same as the desktop script, right?

Yes, it's the same. Sounds good!

The scripts deviated some time ago so this commits aims to unify
the android script with the desktop equivalent in terms of its
structure as well as how it handles versioning and artifacts.
@albin-mullvad albin-mullvad force-pushed the unify-android-build-server-script-with-desktop-droid-197 branch from f274e46 to ca7d8ec Compare September 21, 2023 09:27
@albin-mullvad albin-mullvad merged commit 686e1b3 into main Sep 21, 2023
7 checks passed
@albin-mullvad albin-mullvad deleted the unify-android-build-server-script-with-desktop-droid-197 branch September 21, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants