Build issue outside of GOPATH resulted in file named '..js'. (#699) #702
Conversation
|
Apologies for creating a new pull request. |
tool.go
Outdated
| @@ -143,7 +143,11 @@ func main() { | |||
| } | |||
| if len(pkgs) == 1 { // Only consider writing output if single package specified. | |||
| if pkgObj == "" { | |||
| pkgObj = filepath.Base(pkg.ImportPath) + ".js" | |||
| if pkg.ImportPath == "." { | |||
There was a problem hiding this comment.
I wonder if this would be a problem for the (much less common) case of .. as well?
Why someone would go build .. I have no idea... :) So whether to consider this is entirely up to you.
There was a problem hiding this comment.
Good catch. It's possible for this to happen, I verified. It'd be nice to fix it too.
a reasonable name. In this case the parent directories name rather than "..js" or "...js". The former is a result of building in the current project directory and the latter a result of building in a child directory eg "gopherjs build ..".
tool.go
Outdated
| @@ -143,7 +143,8 @@ func main() { | |||
| } | |||
| if len(pkgs) == 1 { // Only consider writing output if single package specified. | |||
| if pkgObj == "" { | |||
| if pkg.ImportPath == "." { | |||
| importPath := filepath.Base(pkg.ImportPath) | |||
| if importPath == "." || importPath == ".." { | |||
| pkgObj = filepath.Base(currentDirectory) + ".js" | |||
There was a problem hiding this comment.
I don't think this is right. I think for the .. case, we want to use the name of the parent directory.
directory of the project being built for the resulting filename.
tool.go
Outdated
| } else if importPath == ".." { | ||
| // allow for build requests like "build .. or build ../.." | ||
| path := currentDirectory | ||
| count := strings.Count(pkg.ImportPath, "..") |
There was a problem hiding this comment.
I hate to keep commenting on this... It's such a corner case. But I don't think this is right. It would incorrectly count for a path like foo..bar/...
There was a problem hiding this comment.
I was a bit confused by what you might mean, thinking that perhaps there are other ways of building that I am unfamiliar with (not to say there are not), but if you mean to say "gopherjs build over..here/.." then it works for me with the patch as-is. I haven't looked specifically at how pkg.ImportPath is being created but "over..here/.." is simplified to "." prior to my code running so I don't believe there should be issues.
There was a problem hiding this comment.
I think that means your new codepath is never being executed. Because "over..here/.." clearly contains two instances of "..". I also can't get that codepath to execute on my test environment (Linux).
When I execute similar tests, I also get the right output, but I also see that pkg.ImportPath has already had the trailing "/.." removed. In fact, I can't even get gopherjs build .. to break. I expect this is related to the operating system and/or shell I'm using. Since @shurcooL was able to produce the problem with "..", maybe he can provide further details (although I'm pretty sure he's using OSX) and/or test some other ".." combinations.
through a cleaned import path. Each step also steps the current directory up one parent folder until the import path is exhausted. At that point the current directory name is used for the built file.
|
The implementation has grown quite complex, trying to deal with the edge cases one by one. I suspect this can be resolved more simply. Would the following strategy work?
I.e., something like this: if !build.IsLocalImport(pkg.ImportPath) {
pkgObj = path.Base(pkg.ImportPath) + ".js"
} else {
pkgObj = filepath.Base(pkg.Dir) + ".js"
}Any problems with that algorithm? |
There was a problem hiding this comment.
Alternatively, we could simplify this even further to just:
pkgObj = filepath.Base(pkg.Dir) + ".js"The base of dir will always match the base of import path, won't it? Is there any reason not to do that?
Edit: I can only think of it not working if there are symlinks involved. So maybe it's better to use import path after all, when it's available.
tool.go
Outdated
| @@ -143,7 +143,11 @@ func main() { | |||
| } | |||
| if len(pkgs) == 1 { // Only consider writing output if single package specified. | |||
| if pkgObj == "" { | |||
| pkgObj = filepath.Base(pkg.ImportPath) + ".js" | |||
| if !build.IsLocalImport(pkg.ImportPath) { | |||
| pkgObj = filepath.Base(pkg.ImportPath) + ".js" | |||
There was a problem hiding this comment.
Import paths are always separated by a forward slash, never backslash, so we should use path.Base rather than filepath.Base on it. Mind making that change?
|
Okay, this is a simple solution that should work. If we discover concrete cases where it doesn't work (e.g., due to use of symlinks), then we can reintroduce the code that uses base of import path when it's not local, along with a comment explaining why it's needed. So please report any issues as soon as you spot them. Until then, let's enjoy the simpler solution. LGTM. Thanks! |
No description provided.