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

Allow relative paths in GOHACK env var #45

Merged

Conversation

davidovich
Copy link
Contributor

@davidovich davidovich commented Mar 27, 2019

Prior to this change, we were using filepath.Join() to derive a
replacement for the module dir. But this is not enough to
support relative directories, as Join() cleans the resulting path,
removing any "./" prefix (which is needed by the replace drective when
using a local directory).

This change adds the "./" prefix if the user specified it in the GOHACK
env var.

Note that relative parent directory ".." is not cleaned by
filepath.Join(), so we do not need to cater to this case, and an added
test proves that this works.

fixes #44

@davidovich davidovich force-pushed the 44-allow-relative-dir-in-GOHACK-env-var branch from d1abf9a to a4d03d6 Compare March 27, 2019 12:31
mod.go Outdated
if d == "" {
d = filepath.Join(os.Getenv("HOME"), "gohack")
} else {
sep := string(filepath.Separator)
Copy link
Collaborator

@myitcv myitcv Mar 27, 2019

Choose a reason for hiding this comment

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

I might well be missing something here, but don't we simply want to switch based on whether the value of $GOHACK is absolute or not?

If it's absolute, we use it as is.

If not, we join $GOHACK to os.Getwd()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you mean GOENV => GOHACK. I tried to toy with IsAbs, but I had the problem that parent relative dirs are undistinguished from ./ and we need to keep the ./ that is stripped from filepath.Join() so that go does not complain.

ref #44.

If we Join GOHACK and os.Getwd(), we will effectively get an absolute path, and this can interfere with mixed environments (WSL and Windows for example).

I agree that this seems harder than it needs to be, I was stumped on deriving the best approach on a seemingly very simple problem. Do you have other suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you mean GOENV => GOHACK.

Yes, thank you!

In the case the env var does have a value, I was thinking of something like this (untested):

if !filepath.IsAbs(d) {
    d = filepath.Join(cwd, d)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I really got that :-), but that will result in an absolute path, which is not very portable when doing simultaneous dev on Windows and WSL (linux). If we keep the relative path as is (not cleaned by file path), the result works on both environments. Consider this small test env (your implementation), to this (the current PR proposition).

For best interop, it is better to keep the relative dir notation in the module replace.

In other words I would prefer this:

replace github.com/org/module => ./github.com/org/module

to this

replace github.com/org/module => /mnt/c/dev/project/github.com/org/module

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I really got that :-)

My bad, my misunderstanding!

You talk about this "working" cross platform. But I'm not sure if you have a directory replace target that it can ever, by definition, be cross-platform.

So therefore you can't look to checkin a go.mod file that has directory targets, unless you're comfortable with the limitation that the slashes must be one way or the other.

Or is there another use case you had in mind here?

Perhaps adding a bit of details on the steps you're envisaging would help my slow brain 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I'm not sure if you have a directory replace target that it can ever, by definition, be cross-platform.

If you place your source on the Windows filesystem (which is acessible through a WSL Linux mount), relying on the fact that '.' and '..' have the same semantics on Windows and Linux (WSL), it does work (in fact, I use the current gohack and then edit my go.mod by hand to prefix ./). On the other hand, an absolute path definitively make things incompatible.

So therefore you can't look to checkin a go.mod file that has directory targets, unless you're comfortable with the limitation that the slashes must be one way or the other.

Right, and they can be one way or the other (Windows supports / in file paths). But even then, I see gohack as a transient solution to assemble dependencies to be worked on simultaneously in the context of usage. I do not have the present need to check these in, but even if I had, it would work better with relative paths for my co-workers than an absolute path (which would make little sense to share with others, even on the same platform).

I work primarily with VSCode installed on Windows (which calls into a go installed there), but my toolchain (ci, dev cycle) happens on WSL (linux), in a shared filesystem path, so interop is of great value to me (I stay as far to Windows as I can).

Does that add enough (for both our slow brains :-))?

 Prior to this change, we were using `filepath.Join()` to derive a
 replacement for the module dir. But this is not enough to
 support relative directories, as `Join()` cleans the resulting path,
 removing any `"./"` prefix (which is needed by the replace drective when
 using a local directory).

 This change adds the `"./"` prefix if the user specified it in the GOHACK
 env var.

 Note that relative parent directory `".."` is not cleaned by
 `filepath.Join()`, so we do not need to cater to this case, and an added
 test proves that this works.

 fixes rogpeppe#44
@davidovich davidovich force-pushed the 44-allow-relative-dir-in-GOHACK-env-var branch from a4d03d6 to b9f8025 Compare March 29, 2019 14:44
Copy link
Collaborator

@myitcv myitcv left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to discuss @davidovich

@myitcv myitcv merged commit 01736fb into rogpeppe:master Mar 29, 2019
@davidovich davidovich deleted the 44-allow-relative-dir-in-GOHACK-env-var branch March 29, 2019 15:15
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.

gohack does not allow relative directories in GOHACK env var.
2 participants