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

fix(build): Allow update-apps.sh script to work with non-master branches #47351

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

provokateurin
Copy link
Member

Summary

The script only worked for apps which use master as their main branch.
This change also makes the script easier to read and also catch errors properly.

Checklist

Signed-off-by: provokateurin <kate@provokateurin.de>
@provokateurin provokateurin added bug 3. to review Waiting for reviews labels Aug 20, 2024
@provokateurin provokateurin added this to the Nextcloud 31 milestone Aug 20, 2024
@provokateurin provokateurin requested a review from a team August 20, 2024 13:33
@skjnldsv skjnldsv disabled auto-merge August 20, 2024 18:55
@skjnldsv skjnldsv merged commit 4f70998 into master Aug 20, 2024
113 checks passed
@skjnldsv skjnldsv deleted the fix/build/update-apps branch August 20, 2024 18:55
@come-nc
Copy link
Contributor

come-nc commented Aug 26, 2024

Fails with sh:

$ sh build/update-apps.sh
build/update-apps.sh: 18: set: Illegal option -o pipefail

Fails with french locale:

$ bash build/update-apps.sh

activity
fatal: empty string is not a valid pathspec. please use . instead if you meant to match all paths

(because git remote show origin does not contain HEAD branch but a french translation)

@provokateurin
Copy link
Member Author

Fails with sh:

Expected to fail, the shebang clearly says it needs bash.

(because git remote show origin does not contain HEAD branch but a french translation)

Ugh I didn't know and think about that. Not sure how to make that work. I tried a few alternatives, but none of them worked for me at all.

@provokateurin
Copy link
Member Author

For the later we could do a stupid git checkout master || git checkout main. If both fail then the whole script will fail as expected.

@susnux
Copy link
Contributor

susnux commented Aug 26, 2024

Ugh I didn't know and think about that. Not sure how to make that work. I tried a few alternatives, but none of them worked for me at all.

I think in that script just export LANG=C in the beginning should be enough (and should not conflict as long as you do not source it)

@provokateurin
Copy link
Member Author

@come-nc can you try that and if it works make a PR?

@provokateurin
Copy link
Member Author

and should not conflict as long as you do not source it

Instead of using export the env variable could just be applied to the git command that is problematic.

@susnux
Copy link
Contributor

susnux commented Aug 27, 2024

Instead of using export the env variable could just be applied to the git command that is problematic.

Sure this also works :)
I just export to have a consistent scripting env. The env is not leaking into your personal shell session (as long as you do not source it, but why should you? 😅 )

@provokateurin
Copy link
Member Author

Yeah it would be weird to do that, but you mentioned it so we can also just prevent it :D

@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants