Skip to content

Update build script to work with submodules #7864

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

Merged
merged 1 commit into from
Apr 17, 2025

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Mar 21, 2025


This change is Reviewable

@rablador rablador requested a review from pinkisemils March 21, 2025 12:51
Copy link
Collaborator

@pinkisemils pinkisemils 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 r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

    run_git reset --hard
    run_git checkout "$tag"
    run_git submodule update --init --recursive ios/wireguard-apple

We could maybe run this optionally only if ios/wireguard-apple exists, and then I could sleep a bit easier in the night. I think git was failing to find the submodule in the earlier commits where the submodule didn't exist, no?


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

        for tag in "${tags[@]}"; do
          build_ref "refs/tags/$tag"

@albin-mullvad are we certain this wasn't the line that was failing? Since it was

Copy link
Collaborator

@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: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils and @rablador)


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

@albin-mullvad are we certain this wasn't the line that was failing? Since it was

Yes, I'm pretty sure, but as discussed I'll have another look on the build server

Copy link
Contributor Author

@rablador rablador 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 @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

We could maybe run this optionally only if ios/wireguard-apple exists, and then I could sleep a bit easier in the night. I think git was failing to find the submodule in the earlier commits where the submodule didn't exist, no?

Wasn't it the other way around, that it didn't init the submodule?

Copy link
Collaborator

@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: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):
It's indeed the fetch that fails. Here's a minimal way of reproducing on the build server, however I don't get the same on my dev machine (not sure if due to the repository state or environment...):

> git tag | xargs git tag -d && git fetch --prune --tags

...

Fetching submodule ios/wireguard-apple
fatal: remote error: upload-pack: not our ref dcf86e21f8ec9f9c4c135fe98e57471df6ea7989
Errors during submodule fetch:
	ios/wireguard-apple

Since I don't believe we depend on submodules when fetching tags, I added the following which seem to mitigate the issue.

> git tag | xargs git tag -d && git fetch --prune --tags --recurse-submodules=no

So maybe we just add that flag? Might be worth doing even if the underlying cause might be some part of the checked out repository being dirty.

Copy link
Contributor Author

@rablador rablador 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 @pinkisemils)


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

Previously, albin-mullvad wrote…

It's indeed the fetch that fails. Here's a minimal way of reproducing on the build server, however I don't get the same on my dev machine (not sure if due to the repository state or environment...):

> git tag | xargs git tag -d && git fetch --prune --tags

...

Fetching submodule ios/wireguard-apple
fatal: remote error: upload-pack: not our ref dcf86e21f8ec9f9c4c135fe98e57471df6ea7989
Errors during submodule fetch:
	ios/wireguard-apple

Since I don't believe we depend on submodules when fetching tags, I added the following which seem to mitigate the issue.

> git tag | xargs git tag -d && git fetch --prune --tags --recurse-submodules=no

So maybe we just add that flag? Might be worth doing even if the underlying cause might be some part of the checked out repository being dirty.

We would still need the submodule fix though, right?

Copy link
Collaborator

@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: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Wasn't it the other way around, that it didn't init the submodule?

We should probably do it like the other buildserver scripts, e.g:

git submodule update --init wireguard-go-rs/libwg/wireguard-go || true

Note that we seem to run submodule update in two explicit steps per the following PR: #7515


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We would still need the submodule fix though, right?

Yes, this would only improve the tag fetching. The other issue still needs to be addressed.

Copy link
Collaborator

@pinkisemils pinkisemils 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 and @rablador)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, albin-mullvad wrote…

We should probably do it like the other buildserver scripts, e.g:

git submodule update --init wireguard-go-rs/libwg/wireguard-go || true

Note that we seem to run submodule update in two explicit steps per the following PR: #7515

I agree with Albin here. I guess we're not out of the buildserver hell just yet.


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

Previously, albin-mullvad wrote…

Yes, this would only improve the tag fetching. The other issue still needs to be addressed.

The solution you have here is probably the one we want to use then.

@rablador rablador force-pushed the update-build-server-script branch from 82d21bf to a6ffc9d Compare March 25, 2025 07:18
Copy link
Contributor Author

@rablador rablador 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 @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I agree with Albin here. I guess we're not out of the buildserver hell just yet.

Changed


ci/ios/buildserver-build-ios.sh line 101 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

The solution you have here is probably the one we want to use then.

Changed

Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Changed

The above was just and example. Now it's the wrong submodule. How about doing something like the following which is the same pattern we use for other desktop and android submodules?

run_git submodule update
run_git submodule update --init ios/wireguard-apple || true

ci/ios/buildserver-build-ios.sh line 94 at r2 (raw file):

        run_git tag | xargs git tag -d > /dev/null

        run_git fetch --prune --tags 2> /dev/null || continue

Wouldn't the following make more sense? 🤔 In other words just adding the recurse-submodules flag to the command we used before in order to minimize the risk of impacting unrelated things

run_git fetch --prune --tags --recurse-submodules=no 2> /dev/null || continue

Code quote:

run_git fetch --prune --tags 2> /dev/null || continue

Copy link
Contributor Author

@rablador rablador 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 and @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, albin-mullvad wrote…

The above was just and example. Now it's the wrong submodule. How about doing something like the following which is the same pattern we use for other desktop and android submodules?

run_git submodule update
run_git submodule update --init ios/wireguard-apple || true

My bad, I thought this was some specific thing I wasn't aware of. 🙈 I think this last change makes sense.


ci/ios/buildserver-build-ios.sh line 94 at r2 (raw file):

Previously, albin-mullvad wrote…

Wouldn't the following make more sense? 🤔 In other words just adding the recurse-submodules flag to the command we used before in order to minimize the risk of impacting unrelated things

run_git fetch --prune --tags --recurse-submodules=no 2> /dev/null || continue

Bash is really not my first language. So if we fail pruning we skip and do the loop again?

@rablador rablador force-pushed the update-build-server-script branch from a6ffc9d to 5a25581 Compare March 26, 2025 12:44
Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils and @rablador)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

My bad, I thought this was some specific thing I wasn't aware of. 🙈 I think this last change makes sense.

We still don't seem to have the separate run_git submodule update line. There should be two submodule update commands if we want to follow the pattern in other build scripts.


ci/ios/buildserver-build-ios.sh line 94 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Bash is really not my first language. So if we fail pruning we skip and do the loop again?

Yeah, forever and ever without any sleep which isn't optimal, but at the same time it's a bit different from the submodule issue. I'm fine changing from continue to true, but we should probably keep the stderr handling (2> /dev/null) unless we have a good argument for removing that. Also it looks like we have a double space after the --tags flag.

Copy link
Contributor Author

@rablador rablador 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 and @pinkisemils)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, albin-mullvad wrote…

We still don't seem to have the separate run_git submodule update line. There should be two submodule update commands if we want to follow the pattern in other build scripts.

Right, we want the other modules to be updated as well.


ci/ios/buildserver-build-ios.sh line 94 at r2 (raw file):

Previously, albin-mullvad wrote…

Yeah, forever and ever without any sleep which isn't optimal, but at the same time it's a bit different from the submodule issue. I'm fine changing from continue to true, but we should probably keep the stderr handling (2> /dev/null) unless we have a good argument for removing that. Also it looks like we have a double space after the --tags flag.

Alright, so like this then?
run_git fetch --prune --tags --recurse-submodules=no 2> /dev/null || true

@rablador rablador force-pushed the update-build-server-script branch from 5a25581 to bbd667e Compare March 27, 2025 10:52
Copy link
Collaborator

@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.

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils and @rablador)


ci/ios/buildserver-build-ios.sh line 47 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Right, we want the other modules to be updated as well.

👍


ci/ios/buildserver-build-ios.sh line 94 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Alright, so like this then?
run_git fetch --prune --tags --recurse-submodules=no 2> /dev/null || true

Yes, looks good! 👍

Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Dismissed @pinkisemils from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@rablador rablador force-pushed the update-build-server-script branch from bbd667e to dfbc325 Compare April 17, 2025 13:06
@albin-mullvad albin-mullvad dismissed pinkisemils’s stale review April 17, 2025 13:06

Dismissing since we've previously aligned on these changes.

@rablador rablador merged commit 0bc5042 into main Apr 17, 2025
8 checks passed
@rablador rablador deleted the update-build-server-script branch April 17, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants