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(archlinux): .MTREE should have parent dirs as well #645

Merged
merged 5 commits into from
Apr 6, 2023
Merged

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Apr 5, 2023

this walks up the destination and creates the .MTREE entries for the parents too, otherwise packages are not installable by pacman.

if anyone knows a better way to do this, I'm all ears...

closes #638

Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
@caarlos0 caarlos0 added the bug Something isn't working label Apr 5, 2023
@caarlos0 caarlos0 requested a review from erikgeiser April 5, 2023 13:14
@caarlos0 caarlos0 self-assigned this Apr 5, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2023
@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #645 (99913b1) into main (5d25139) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
- Coverage   75.69%   75.68%   -0.01%     
==========================================
  Files          14       14              
  Lines        2942     2941       -1     
==========================================
- Hits         2227     2226       -1     
  Misses        506      506              
  Partials      209      209              
Impacted Files Coverage Δ
arch/arch.go 78.57% <100.00%> (-0.05%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@erikgeiser
Copy link
Member

erikgeiser commented Apr 5, 2023

I can’t really review this PR right now but from a first glance I think this should be fixed by taking advantage of the new content api I introduced. There we create implicit directories (a dedicated content type) that we add for debs but not for rpms. So arch packages should work like debs in this regard.

@erikgeiser
Copy link
Member

erikgeiser commented Apr 5, 2023

We probably just need to add implicit dirs in the content type switch in WriteTo. Currently the parent dirs are probably added as regular file entries due to the default case.

@erikgeiser
Copy link
Member

This should do the trick:

 func (me *MtreeEntry) WriteTo(w io.Writer) (int64, error) {
        switch me.Type {
-   case files.TypeDir:
+ case files.TypeDir, files.TypeImplicitDir:
                n, err := fmt.Fprintf(
                        w,
                        "./%s time=%d.0 mode=%o type=dir\n",

However, the test does take the info.Contents into account. In my opinion, the TestArchMtree test should not check for implicit directories. We should add another test, that builds the entire package and then checks if the mtree contains implicit directories.

I'm sorry, but I don't have the time to rewrite the test, otherwise I would have already pushed the diff above.

@caarlos0
Copy link
Member Author

caarlos0 commented Apr 5, 2023

This should do the trick:

 func (me *MtreeEntry) WriteTo(w io.Writer) (int64, error) {
        switch me.Type {
-   case files.TypeDir:
+ case files.TypeDir, files.TypeImplicitDir:
                n, err := fmt.Fprintf(
                        w,
                        "./%s time=%d.0 mode=%o type=dir\n",

However, the test does take the info.Contents into account. In my opinion, the TestArchMtree test should not check for implicit directories. We should add another test, that builds the entire package and then checks if the mtree contains implicit directories.

I'm sorry, but I don't have the time to rewrite the test, otherwise I would have already pushed the diff above.

ah way better indeed, will work on it, thanks!

Co-Authored-By: Co-authored-by: Erik Geiser <erik.geiser@posteo.net>
Signed-off-by: Carlos A Becker <caarlos0@users.noreply.github.com>
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 5, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 99913b1
Status: ✅  Deploy successful!
Preview URL: https://cf9c84f8.nfpm.pages.dev
Branch Preview URL: https://mtree-tree.nfpm.pages.dev

View logs

@caarlos0
Copy link
Member Author

caarlos0 commented Apr 5, 2023

@erikgeiser fwiw that doesn't work, will look into why later... but the root folders are still missing there...

@erikgeiser
Copy link
Member

Yeah I missed this one:

if content.Type == files.TypeDir {

The mtree entry should always be added.

@caarlos0
Copy link
Member Author

caarlos0 commented Apr 5, 2023

Ah right, I'm 100% blind 😂

Co-Authored-By: Co-authored-by: Erik Geiser <erik.geiser@posteo.net>
Signed-off-by: Carlos A Becker <caarlos0@users.noreply.github.com>
@caarlos0
Copy link
Member Author

caarlos0 commented Apr 6, 2023

ight, all fixed, thanks for the help @erikgeiser

Signed-off-by: Carlos A Becker <caarlos0@users.noreply.github.com>
@caarlos0 caarlos0 merged commit 3f95279 into main Apr 6, 2023
@caarlos0 caarlos0 deleted the mtree-tree branch April 6, 2023 02:51
@github-actions github-actions bot added this to the v2.27.0 milestone Apr 6, 2023
@caarlos0 caarlos0 modified the milestones: v2.27.0, v2.28.0 Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

archlinux package produces warnings when installed
2 participants