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

feat: initial commit for erofs support #14

Closed
wants to merge 2 commits into from

Conversation

rchincha
Copy link

@rchincha rchincha commented Sep 6, 2024

No description provided.

@rchincha rchincha force-pushed the erofs branch 3 times, most recently from 0abac16 to 2843a67 Compare September 30, 2024 21:37
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 16.44833% with 574 lines in your changes missing coverage. Please review.

Project coverage is 16.28%. Comparing base (ad93e06) to head (c01a838).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
erofs/erofs.go 7.48% 363 Missing and 8 partials ⚠️
erofs/verity.go 20.74% 185 Missing and 6 partials ⚠️
erofs/superblock.go 77.77% 4 Missing and 4 partials ⚠️
erofs/mediatype.go 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   16.52%   16.28%   -0.25%     
==========================================
  Files           6       10       +4     
  Lines         932     1640     +708     
==========================================
+ Hits          154      267     +113     
- Misses        757     1334     +577     
- Partials       21       39      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ramkumar Chinchani <[email protected]>
@rchincha rchincha force-pushed the erofs branch 2 times, most recently from c27c15d to 29321f8 Compare October 3, 2024 21:06
We are going to support multiple underlying filesystems (squash, erofs
and maybe more). As long as they are filesystem image blobs, verity data
can be appended.

Signed-off-by: Ramkumar Chinchani <[email protected]>
erofs/erofs.go Outdated
type erofsFuseInfoStruct struct {
Path string
Version string
SupportsNotfiy bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typo is consistent but will be murder to maintain later :)

erofs/erofs.go Outdated
// everyting underneath is also implicitly excluded. The
// AddExclude()/AddInclude() methods do the math to figure out what is the
// correct set of things to exclude or include based on what paths have been
// previously included or excluded.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding this hard to reason about, and therefore finding it hard to figure out whether AddExclude() is doing the right thing.

I think you're mainly thinking in terms of "I am generating a changeset and therefore want to include only files which have changed", is that right?

erofs/erofs.go Outdated
// If /usr/bin/ls has changed but /usr hasn't, we don't want to list
// /usr in the include paths any more, so let's be sure to only
// add things which aren't prefixes.
if strings.HasPrefix(inc, p) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if both /usr/bin/cal and /usr/bin/calendar have changed? Will this check end up possibly ignoring /usr/bin/cal?

erofs/erofs.go Outdated
eps.include = append(eps.include, orig)
}

func (eps *ExcludePaths) String() (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are using this only to spit out a file to feed to mkerofs. But because it is generally called String(), I would expect it to be more generally useful, so in particular would expect it to also print the includes.

I'm not sure the best way to handle this. Probably best to just leave a comment above it explaining that some information does get lost and why.

erofs/erofs.go Outdated
return blob, GenerateErofsMediaType(compression, verity), rootHash, nil
}

func isMountedAtDir(_, dest string) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is checking that the source is correct a TODO?

@rchincha
Copy link
Author

PR #25 supersedes this one, so closing.

@rchincha rchincha closed this Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants