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

switch to use packr to embed static assets #5952

Open
wants to merge 12 commits into
base: master
from

Conversation

@techknowlogick
Copy link
Member

techknowlogick commented Feb 4, 2019

Fix #4250

@lafriks

lafriks approved these changes Feb 4, 2019

@GiteaBot GiteaBot added the lgtm/need 1 label Feb 4, 2019

@techknowlogick techknowlogick requested a review from lunny Feb 4, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 4, 2019

Codecov Report

Merging #5952 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5952      +/-   ##
==========================================
- Coverage   38.85%   38.84%   -0.02%     
==========================================
  Files         352      352              
  Lines       50045    50045              
==========================================
- Hits        19447    19439       -8     
- Misses      27782    27791       +9     
+ Partials     2816     2815       -1
Impacted Files Coverage Δ
modules/options/options.go 96.15% <ø> (ø) ⬆️
modules/public/public.go 74.41% <ø> (ø) ⬆️
models/unit.go 0% <0%> (-14.29%) ⬇️
routers/repo/view.go 44.54% <0%> (-1.15%) ⬇️
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️

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 eaf9ded...16c36c7. Read the comment docs.

@zeripath
Copy link
Contributor

zeripath left a comment

Need to change the snap/snapcraft.yaml too.

I can't believe that go-bindata isn't more clearly reported as required when building from source but it looks like it's not really mentioned. I think we need to add something to the documentation to say we're no longer using go-bindata and have moved to packr with URL - explaining that it should just be downloaded as necessary.

@techknowlogick

This comment has been minimized.

Copy link
Member Author

techknowlogick commented Feb 4, 2019

Good idea @zeripath. I've updated snap, and in terms of documentation I agree. I think https://docs.gitea.io/en-us/hacking-on-gitea/ needs more information overall, especially in regards to use of make file (in addition to exactly how bindata is used).

If you are just waking up now that means it means I should've been asleep hours ago. If you want to get a head start on documentation feel free to push directly to my branch, otherwise I will add some when I wake up. Good night 😴

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 4, 2019

hmm... So I just pulled this repo and tried to build:

Error: open /home/andrew/go/src/code.gitea.io/gitea/templates/repo/settings/deploy_keys.tmpl: too many open files

It's also extremely verbose whilst building...

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 4, 2019

This too many open files problem is rather distasteful - it's a packing program why does it need to have >1000 file descriptors open?

techknowlogick added some commits Feb 5, 2019

@techknowlogick

This comment has been minimized.

Copy link
Member Author

techknowlogick commented Feb 5, 2019

@zeripath I've reduced the number of files open required, let me know if this works for you.

techknowlogick added some commits Feb 5, 2019

@techknowlogick

This comment has been minimized.

Copy link
Member Author

techknowlogick commented Feb 5, 2019

@zeripath docs added

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 5, 2019

Thanks @techknowlogick . I'm still getting a too many files open error.

The default ulimit on ubuntu is:

$ ulimit -n
1024

It really shouldn't be having open that many file-descriptors! There's something not right in packr I think.

What's your ulimit -n? Is it wildly higher?

@techknowlogick

This comment has been minimized.

Copy link
Member Author

techknowlogick commented Feb 5, 2019

Thanks for checking again 😄 I really appreciate it.

ulimit -n is 4864 for me. I think packr is trying to parallelize packing the files by loading as many as it can at once. However, as 1024 is the default for ubuntu I'm going to set this PR as blocked as I don't want to tell potential devs to increase the file descriptor limit to be able to contribute to Gitea.

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 5, 2019

I'm still shocked that it's doing this. I'm fairly certain that it must be leaking file descriptors somewhere.

Looking on my phone I've seen one place in decompress that's leaking fds: https://github.com/gobuffalo/packr/blob/b0dc8ae47dc76a183765618fbece41cd433a8e2c/box.go#L127

This reader isn't being closed...

But I don't know if that is the cause. I'd need to recompile and see.

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.x.x Feb 8, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor

zeripath commented Feb 10, 2019

Hmm. OK so I upgraded to go 1.11 and this problem has disappeared...

I'm not sure whether that makes this a blocker or not?


I spoke too soon... It's still there.

techknowlogick added some commits Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment