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

Build issue outside of GOPATH resulted in file named '..js'. (#699) #702

Merged
merged 9 commits into from Sep 27, 2017

Conversation

Projects
None yet
3 participants
@4ydx
Contributor

4ydx commented Sep 14, 2017

No description provided.

@4ydx

This comment has been minimized.

Show comment
Hide comment
@4ydx

4ydx Sep 14, 2017

Contributor

Apologies for creating a new pull request.

Contributor

4ydx commented Sep 14, 2017

Apologies for creating a new pull request.

@dmitshur

LGTM. I don't have any suggestions for how to improve this.

Thanks (and no worries about making a 2nd PR)!

Show outdated Hide outdated tool.go
When building outside the GOPATH make sure that the built file will have
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 ..".
Show outdated Hide outdated tool.go
Build requests like "gopherjs build ../.." will now use the parent
directory of the project being built for the resulting filename.
Show outdated Hide outdated tool.go

4ydx added some commits Sep 22, 2017

The filename used for the build js file is now found by stepping back
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.
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 25, 2017

Member

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?

  1. Check if import path is local using build.IsLocalImport.

  2. If not, use path.Base as before.

  3. If local, use filepath.Base of pkg.Dir.

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?

Member

dmitshur commented Sep 25, 2017

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?

  1. Check if import path is local using build.IsLocalImport.

  2. If not, use path.Base as before.

  3. If local, use filepath.Base of pkg.Dir.

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?

@dmitshur

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.

Show outdated Hide outdated tool.go
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Sep 27, 2017

Member

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!

Member

dmitshur commented Sep 27, 2017

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!

@dmitshur dmitshur merged commit 4152256 into gopherjs:master Sep 27, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment