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

--embed with no alias drops the parent directory from the embedded files #202

Closed
camhux opened this issue Sep 14, 2024 · 6 comments
Closed

Comments

@camhux
Copy link
Contributor

camhux commented Sep 14, 2024

When xcaddy is invoked with an --embed target without an alias, referring to a directory within the working directory, like so:

$ xcaddy --embed my-dir

…this results in the files/ directory of Caddy's embedded FS containing the contents of my-dir/, as though my-dir/* were the target, rather than a files/my-dir/ directory containing the contents of my-dir.

This behavior was unexpected for me because it diverges from the way that the go:embed directive of the Go tool treats its path arguments. Go will embed a specified directory (without a glob suffix) as a directory, and any specified subdirectories will preserve the entire directory structure that leads to that subdirectory. I.e., go:embed my-dir creates an embedded directory my-dir, and go:embed dir-a/dir-b creates an embedded directory structure of dir-a/dir-b.

This behavior results from this line:

err = copy(d.Dir, filepath.Join(tempFolder, "files", d.Name))

where d.Name is empty if there was no alias in the corresponding argument to --embed.


I see how this could be the intended design for --embed, and I also realize that the argument to --embed must not be treated identically as the arguments to a go:embed directive, since relative path elements like .. are permitted in --embed arguments for greater flexibility. If this is the case, I think this behavior should be specified in the --embed documentation.

If it's not the intended design, I suggest that an unaliased path passed to --embed that points to a directory should use the last directory name as the copy target when creating the entry in the embedded files/ dir. I.e., --embed ../other-dir/my-dir -> files/my-dir.

Either way, I'm happy to submit the patch. (I've been tinkering with --embed on a fork already.)

Thanks!

@mohammed90
Copy link
Member

The logic is a carryover from Matthew's prototype module https://github.com/mholt/caddy-embed. He lays out the reasoning for that part in the README under the Site Root section.

@mholt
Copy link
Member

mholt commented Sep 16, 2024

Hmm, yeah, I think the reason for this is to make it so that URI paths don't have to contain a subfolder. The problem is that without the way we do it, you have to have a subfolder like /files/... in your URI, whereas with the way we do it, you may have a subfolder if you want, by naming it anything you like, within the source folder. Does that make sense?

I mean, if this is truly a problem I am open to changing things, I just figured people would want the option to choose whether a subfolder was required or not.

@mohammed90
Copy link
Member

I think using root is helpful to avoid having the subfolder in the URI. There's one example where it's used in your repo, and it can be omitted to default to whatever root it has.

@camhux
Copy link
Contributor Author

camhux commented Sep 16, 2024

Thanks for the context! Right, I see how this came to behave the way it does. To be clear, I was motivated to ask about it from a UX perspective (especially as a divergence from go:embed path handling), not because this behavior is blocking anything I'm trying to do. I wasn't seeing the need for embedding dir contents directly since I've only been using Caddy directives that specify a root.

Given all that, and that the alias syntax for the --embed flag is an obvious way to embed a directory itself, I don't think there's any reason to change this. I do think it's worth clarifying this behavior in the README's documentation of --embed (and maybe the command helptext too) just to reduce ambiguity for other newcomers.

Want me to send a tiny docs PR based on this discussion?

@mholt
Copy link
Member

mholt commented Sep 18, 2024

Yes a simple PR would be accepted! Thank you!

@camhux
Copy link
Contributor Author

camhux commented Sep 23, 2024

Did wordsmithing here: #206. Whether or not the PR goes in, I consider this issue resolved. Thanks!

@camhux camhux closed this as completed Sep 23, 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

No branches or pull requests

3 participants