Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Ensure we read from overlay when calling readDir #786

Closed
wants to merge 3 commits into from
Closed

Ensure we read from overlay when calling readDir #786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
70bee89
Ensure we read from overlay when hitting readDir
tonyhb Feb 21, 2021
bce2f82
Sort slices so that the order of output is deterministic
tonyhb Feb 21, 2021
60d8365
Enable overlay filesystems for webassembly
tonyhb Feb 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions cue/load/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,20 @@ func hasSubdir(root, dir string) (rel string, ok bool) {

func (fs *fileSystem) readDir(path string) ([]os.FileInfo, errors.Error) {
path = fs.makeAbs(path)

if fi := fs.getDir(path, false); fi != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a fs.getDir call below that is ostensibly there to achieve the same. So if the below code is buggy, it should be fixed there.

Copy link
Author

Choose a reason for hiding this comment

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

This is one of the difficulties when lacking the fs.DirEntry interface. We call ReadDir to iterate through, but this means we must have a concrete directory to read from. This breaks webassembly, as there is no underlying directory to read. This is why we must special case unless we comform to fs.DirEntry and support only go > 1.16.

I do think that @myitcv was correct regarding this patch - it fixes this code such that we use overlays if possible - but the underlying root is the lack of io/fs support.

To be honest, I'm fairly blocked on supporting cue with embedded packages without these fixes. I really understand that we should strive towards an io/fs driver with interfaces - that's the correct solution here.

As you can probably tell, I'm not familiar with cue's internals enough to take this on - I could give it a shot, but there will be lots of feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then my suggestion is that we close this PR and move discussion to #607 or another issue if that one is not appropriate. In those issues, let's focus on specific use cases to help shape the solution.

Copy link
Author

Choose a reason for hiding this comment

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

This PR aims to fix the bugs in #607 - we might want to start an entirely new discussion on the architecture of overlays, files, and cue loading using io/fs as the interface to account for windows, linux, and webassembly (virtual fs) usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR aims to fix the bugs in #607

Per my comments in #786 (comment), I'm not clear that the behaviour described in #607 is actually a bug (the issue itself is ostensibly a feature request). I'll comment on #607 to see if we can find a way forward.

items := make([]os.FileInfo, len(fi))
i := 0
for _, of := range fi {
items[i] = of
i++
}
sort.Slice(items, func(i, j int) bool {
return items[i].Name() < items[j].Name()
})
return items, nil
}

m := fs.getDir(path, false)
items, err := ioutil.ReadDir(path)
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions cue/load/import.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ func (l *loader) importPkg(pos token.Pos, p *build.Instance) []*build.Instance {

found := false
for _, d := range dirs {
if d[1] == "/" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this also work for Windows volumes and other root indicators?

found = true
break
}
info, err := ctxt.stat(d[1])
if err == nil && info.IsDir() {
found = true
Expand Down
4 changes: 4 additions & 0 deletions internal/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"

"github.com/cockroachdb/apd/v2"
Expand Down Expand Up @@ -414,6 +415,9 @@ func IsEllipsis(x ast.Decl) bool {

// GenPath reports the directory in which to store generated files.
func GenPath(root string) string {
if runtime.GOOS == "js" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would "js" have to be special-cased here?

Is it because avoiding picking up the "pkg" directory? This really should be disabled for all OSes, but probably only after a 3.0 release. Is there any reason why JS is special here? If so, please add a comment/ TODO.

return filepath.Join(root, "cue.mod", "gen")
}
info, err := os.Stat(filepath.Join(root, "cue.mod"))
if os.IsNotExist(err) || !info.IsDir() {
// Try legacy pkgDir mode
Expand Down