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

add a directory prefix gitea-src-VERSION to release-tar-file #19396

Merged
merged 5 commits into from
Apr 25, 2022
Merged

add a directory prefix gitea-src-VERSION to release-tar-file #19396

merged 5 commits into from
Apr 25, 2022

Conversation

jklippel
Copy link
Contributor

@jklippel jklippel commented Apr 13, 2022

Do not package tar file such that the sources are unpacked into the
current working directory. Use a directory prefix instead on creating
the tar file.

Fixes: #19066

⚠️ BREAKING ⚠️

Prior to this PR the gitea src tar.gz would extract to the current directory. Users requiring the previous behaviour should use tar options as appropriate.

@techknowlogick
Copy link
Member

Thanks for this PR, could you upload a tar generated with the above? I'd love to pass it to downstream folk (nixos, FreeBSD/OpenBSD,and others) who package gitea using this generated file so we can ensure that if it breaks their packaging it can be resolved prior to a release.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 13, 2022
@wxiaoguang
Copy link
Contributor

@jklippel
Copy link
Contributor Author

Packaged tag v1.16.5 by applying the proposed changes to the Makefile.
gitea-src-1.16.5.tar.gz

@techknowlogick
Copy link
Member

@jklippel thanks for uploading that sample, I've reached out to the *BSD folk and am awaiting feedback, as for nix I've done some preliminary testing and the build will need some changes if/when this is merged.

@wxiaoguang I think this is the better PR because it looks for bsdtar vs tar.

cc: @Ma27 @kolaente (as other nix packagers for gitea)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 14, 2022

@wxiaoguang I think this is the better PR because it looks for bsdtar vs tar.

However it does nothing with bsdtar, there is -s for bsdtar to add the prefix.

Either we should stop the make with an error when there is bsdtar, or we should use bsdtar with -s to make the output has the same prefix. Otherwise there will be inconsistent output on different systems.

@jklippel
Copy link
Contributor Author

@wxiaoguang I think this is the better PR because it looks for bsdtar vs tar.

However it does nothing with bsdtar, there is -s for bsdtar to add the prefix.

Either we should stop the make with an error when there is bsdtar, or we should use bsdtar with -s to make the output has the same prefix. Otherwise there will be inconsistent output on different systems.

I'm in favour of using -s. I will look into it. Is there any specific version of bsdtar or *bsd this should work on?

@wxiaoguang
Copy link
Contributor

I am not a BSD user 😂 I think we can assume that almost all bsdtar have the -s option.

@jklippel
Copy link
Contributor Author

gitea-src-1.16.5.tar.gz
Implemented -s for bsd tar
Implemented and tested using freebsd 12.

Do not package tar file such that the sources are unpacked into the
current working directory. Use a directory prefix instead on creating
the tar file.

Fixes: #19066
@kevans91
Copy link
Contributor

Thanks for this PR, could you upload a tar generated with the above? I'd love to pass it to downstream folk (nixos, FreeBSD/OpenBSD,and others) who package gitea using this generated file so we can ensure that if it breaks their packaging it can be resolved prior to a release.

FWIW- I don't maintain FreeBSD's port, but this change will in-fact simplify the port slightly by aligning to what the ports framework typically expects from a distfile. This is likely a net -1 line change for us, and not one that's going to cause any problems.

@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 Apr 15, 2022
@wxiaoguang wxiaoguang added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/distribution This PR changes something about the packaging of Gitea topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile and removed topic/distribution This PR changes something about the packaging of Gitea labels Apr 15, 2022
@wxiaoguang wxiaoguang changed the title add a directory prefix to release-tar-file add a directory prefix gitea-src-VERSION to release-tar-file Apr 15, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Apr 15, 2022
@silverwind
Copy link
Member

silverwind commented Apr 15, 2022

Is there any specific version of bsdtar or *bsd this should work on?

I am not a BSD user 😂 I think we can assume that almost all bsdtar have the -s option.

The version on macOS should be supported for release creation and it does not have -s. FreeBSD tar indicates it does not have it either.

If everything fails, we could just require GNU tar on macOS viagtar, available via brew install gnu-tar. Thought I would prefer a solution that would work with regular bsdtar, why not just mkdir and cp before tar?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 15, 2022

The version on macOS should be supported for release creation and it does not have -s. FreeBSD tar indicates it does not have it either.

macOS tar does have the -s, check your man tar please.

If everything fails, we could just require GNU tar on macOS viagtar, available via brew install gnu-tar. Thought I would prefer a solution that would work with regular bsdtar, why not just mkdirand move files before taring them?

Why are you reading a FreeBSD 6.0 document, which was released in 2005 ....... FreeBSD since 8 (since 2009) does have it. https://www.freebsd.org/cgi/man.cgi?query=bsdtar&apropos=0&sektion=1&manpath=FreeBSD+8.0-RELEASE&arch=default&format=html

@silverwind
Copy link
Member

Ah, I missed it man bsdtar actually has it on macOS.

bsdtar 3.5.1 - libarchive 3.5.1 zlib/1.2.11 liblzma/5.0.5 bz2lib/1.0.8
     -s pattern
             Modify file or archive member names according to pattern.  The pattern has
             the format /old/new/[ghHprRsS] where old is a basic regular expression, new
             is the replacement string of the matched part, and the optional trailing
             letters modify how the replacement is handled.  If old is not matched, the
             pattern is skipped.  Within new, ~ is substituted with the match, \1 to \9
             with the content of the corresponding captured group.  The optional trailing
             g specifies that matching should continue after the matched part and stop on
             the first unmatched pattern.  The optional trailing s specifies that the
             pattern applies to the value of symbolic links.  The optional trailing p
             specifies that after a successful substitution the original path name and
             the new path name should be printed to standard error.  Optional trailing H,
             R, or S characters suppress substitutions for hardlink targets, regular
             filenames, or symlink targets, respectively.  Optional trailing h, r, or s
             characters enable substitutions for hardlink targets, regular filenames, or
             symlink targets, respectively.  The default is hrs which applies
             substitutions to all names.  In particular, it is never necessary to specify
             h, r, or s.

Why are you reading a FreeBSD 6.0 document

It was the first result that came up in google. It's fine if the argument was added in later versions.

@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 Apr 19, 2022
Makefile Outdated Show resolved Hide resolved
Co-authored-by: silverwind <me@silverwind.io>
@jklippel
Copy link
Contributor Author

Attached files built with branch and latest commit (93e4cf8):
freebsd-gitea-src-1.16.5.tar.gz
gitea-src-1.16.5.tar.gz

@codecov-commenter
Copy link

Codecov Report

Merging #19396 (bb317eb) into main (ebb2396) will decrease coverage by 0.00%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##             main   #19396      +/-   ##
==========================================
- Coverage   47.42%   47.42%   -0.01%     
==========================================
  Files         951      951              
  Lines      132360   132327      -33     
==========================================
- Hits        62767    62750      -17     
+ Misses      62032    62022      -10     
+ Partials     7561     7555       -6     
Impacted Files Coverage Δ
models/issue_label.go 68.46% <0.00%> (+0.44%) ⬆️
routers/web/repo/issue.go 36.74% <75.00%> (+0.02%) ⬆️
models/issue.go 54.14% <100.00%> (+0.05%) ⬆️
modules/indexer/issues/indexer.go 53.70% <100.00%> (ø)
modules/validation/helpers.go 64.44% <100.00%> (-9.33%) ⬇️
routers/api/v1/repo/issue.go 54.68% <100.00%> (+0.05%) ⬆️
routers/web/user/home.go 64.38% <100.00%> (+0.35%) ⬆️
services/migrations/gitea_uploader.go 57.74% <100.00%> (ø)
modules/queue/queue_disk.go 60.00% <0.00%> (-3.34%) ⬇️
modules/queue/queue_disk_channel.go 60.83% <0.00%> (-1.25%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddbbe6e...bb317eb. Read the comment docs.

@wxiaoguang wxiaoguang merged commit 257cea6 into go-gitea:main Apr 25, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 26, 2022
* giteaofficial/main:
  Add a new menu in file view to open blame view and fix blame view select range bug (go-gitea#19500)
  Fix two UI bugs: JS error in imagediff.js, 500 error in diff/compare.tmpl
  [skip ci] Updated translations via Crowdin
  Improve Stopwatch behavior (go-gitea#18930)
  Pass gitRepo down to GetRawDiff, since its used for main repo and wiki (go-gitea#19461)
  Use queue instead of memory queue in webhook send service (go-gitea#19390)
  add a directory prefix `gitea-src-VERSION` to release-tar-file (go-gitea#19396)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…tea#19396)

Use a directory prefix instead on creating the tar file. Fixes: go-gitea#19066
@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tarbomb in release src tarball file
10 participants