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.

Sign up for GitHub

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

Jump to bottom

Improve sed detection in update-locales.sh #23254

Merged
merged 6 commits into from
Mar 4, 2023
Merged

Improve sed detection in update-locales.sh #23254

merged 6 commits into from
Mar 4, 2023

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 2, 2023 •
edited
Loading

  • Make scripts work from any directory
  • Detect sed version just like Makefile does

@silverwind
Improve build scripts
26c39e5 
- Make scripts work from any directory
- Detect sed version just like Makefile does
@silverwind silverwind added type/enhancement An improvement of existing functionality topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile labels Mar 2, 2023
@silverwind silverwind mentioned this pull request Mar 2, 2023
Merged
@silverwind
Copy link
Member Author

silverwind commented Mar 2, 2023 •
edited
Loading

Had to temporarily disable -e because BSD sed will exit non-zero for a --version argument, in fact it has no way to show its version:

$ /usr/bin/sed --version; echo $?
/usr/bin/sed: illegal option -- -
usage: sed script [-Ealnru] [-i extension] [file ...]
  sed [-Ealnu] [-i extension] [-e script] ... [-f script_file] ... [file ...]
1

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2023
@silverwind
Copy link
Member Author

Removed the cd related changes. The scripts run under /bin/sh so bash variables were not there and I'm not 100% sure I can switch these scripts to bash because it's not present in alpine for example. So this is only the sed change now.

@silverwind silverwind changed the title Improve build scripts Improve sed detection in update-locales.sh Mar 2, 2023
@silverwind silverwind added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed type/enhancement An improvement of existing functionality labels Mar 3, 2023
@wxiaoguang
Copy link
Contributor

I didn't know where this script runs in. If it runs in the environment without bash (really? it's 2023 now ...), I think we should also revert the shebang line to #!/bin/sh.

ps: I didn't see today's crowdin sync commit, does it mean that this script failed because I used bash in last PR?
I want to mention some maintainers but I don't know who knows the crwodin sync infrastructure ....

@lunny
Copy link
Member

lunny commented Mar 3, 2023

I think it's copied from Makefile and it works well before with no gsed installation requirement.

@silverwind
Copy link
Member Author

Alpine is pretty much the only environment in use today that does not have bash unless manually installed. I also don't know where/how this script runs. Maybe @techknowlogick has some insight.

wxiaoguang
wxiaoguang reviewed Mar 3, 2023
View reviewed changes
build/update-locales.sh Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

I think we need to take this fix with the shebang line fix .... 😂

@lunny @wxiaoguang
Apply suggestions from code review
3218c08 
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 3, 2023
@silverwind
Copy link
Member Author

As I suspected, this step is running on alpine:3.17, so it needs sh (busybox). I hope the script is sh-compatible.

@silverwind
Copy link
Member Author

Need to merge this to un-break the translation updates.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 4, 2023
@zeripath zeripath merged commit b6d2c94 into go-gitea:main Mar 4, 2023
@silverwind silverwind deleted the buildscripts branch March 4, 2023 14:32
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 6, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
306ba96 
* giteaofficial/main: (28 commits)
  Update hacking-on-gitea-zh_cn documentation (go-gitea#23315)
  Fix broken code editor diff preview (go-gitea#23307)
  [skip ci] Updated translations via Crowdin
  Add context when rendering labels or emojis (go-gitea#23281)
  Change interactiveBorder to fix popup preview  (go-gitea#23169)
  Improve the frontend guideline (go-gitea#23298)
  Scoped labels: set aria-disabled on muted Exclusive option for a11y (go-gitea#23306)
  Add basic documentation for labels, including scoped labels (go-gitea#23304)
  [skip ci] Updated translations via Crowdin
  Re-add accidentally removed `hacking-on-gitea.zh-cn.md` (go-gitea#23297)
  Add default owner team to privated_org and limited_org in unit test (go-gitea#23109)
  Improve sed detection in update-locales.sh (go-gitea#23254)
  Support sanitising the URL by removing extra slashes in the URL (go-gitea#21333)
  Make Ctrl+Enter submit a pending comment (starting review) instead of submitting a single comment (go-gitea#23245)
  Avoid panic caused by broken payload when creating commit status (go-gitea#23216)
  Add run status in action view page (go-gitea#23212)
  update to mermaid v10 (go-gitea#23178)
  Fix code wrap for unbroken lines (go-gitea#23268)
  Fix stray backticks appearing in pull request timeline (go-gitea#23282)
  Fill head commit to in payload when notifying push commits for mirroring (go-gitea#23215)
  ...
@delvh delvh added this to the 1.20.0 milestone Apr 3, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@jolheiser jolheiser jolheiser approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
7 participants
@silverwind @wxiaoguang @lunny @jolheiser @zeripath @GiteaBot @delvh