search

openllb / hlb

A developer-first language to build and test any software efficiently
https://openllb.github.io/hlb/
Apache License 2.0
108 stars 12 forks source link

ModuleDir function returns incorrect result #332

Closed aaronlehmann closed 2 years ago

aaronlehmann commented 2 years ago

This line of code appears to be wrong: https://github.com/openllb/hlb/blob/12055ea32a30db84c267c4d355ef198c2e2080fb/codegen/context.go#L76

mod.Directory.Path() is hardcoded to . by parser.Parse. Normally this strings.TrimPrefix does nothing, but if the filename is ./foo.hlb, strings.TrimPrefix will remove the ., and ModuleDir will return /. This will make a "download "./build" get an error like:

ERROR: rpc error: code = Unknown desc = failed to create synctarget dest dir /build: mkdir /build: permission denied

I haven't been able to reproduce this with the hlb CLI, but passing in a filename starting with ./ into command.Run triggers this issue.

I'm not sure what this line of code is trying to accomplish. Should it just be removed, or is the correct fix something else?

hinshun commented 2 years ago

I think this is to trim the prefix for remote directories, and for local directories it's supposed to be a no-op.

aaronlehmann commented 2 years ago

I think this is to trim the prefix for remote directories, and for local directories it's supposed to be a no-op.

Would this give us a reasonable localPath path for a download included in a remote module?

aaronlehmann commented 2 years ago

@hinshun: Happy to work on a fix for this issue if you can point me in the right direction.

hinshun commented 2 years ago

@aaronlehmann So there is no reproduction with just hlb?

I think was relying on the no-op behaviour, perhaps we should make that more explicit. We have to ensure fixing this doesn't break the different remote module edge cases (like remote module importing ./rel-path within its remote fs, and remote module importing another remote module).

aaronlehmann commented 2 years ago

That's correct. I can't reproduce it with the HLB CLI.

By explicit, do you mean that we'd trip a prefix for remote directories, but not for local directories?

hinshun commented 2 years ago

That's one way to make it explicit. Since Directory is an interface, was hoping we don't type check it or come up with a better way to handling it.

aaronlehmann commented 2 years ago

I still don't understand why the behavior would be correct for remote module dirs. If a remote module has download "./build", don't we want to download to "./build", not some prefix based on where the HLB file is in the remote module?

hinshun commented 2 years ago

I wanted consistency where . is always relative to the module's "directory". For the behavior you describe where you want download in remote module to your local fs, you would need download "${localCwd}/build".

Happy to debate this point, but it was what I ended up with after thinking about it for a while.

hinshun commented 2 years ago

Maybe only functions like export artifacts is always relative to user's cwd. But I wish we can make it clearer... In some of my notes, I had hoped functions like download are in an namespace like local.download which will make me happier.

aaronlehmann commented 2 years ago

Maybe the cleanest solution here is for parser.Parse to set the local directory Path to "" instead of ".". The "." looks arbitrary, and isn't actually a prefix we want to strip off.