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.

Sign up for GitHub

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

Jump to bottom

all: run Windows CI #323

Closed
zombiezen opened this issue Aug 14, 2018 · 6 comments
Closed

all: run Windows CI #323

zombiezen opened this issue Aug 14, 2018 · 6 comments
Assignees
@vangent
Labels
enhancement New feature or request
Milestone
Sprint 19

Comments

@zombiezen
Copy link
Contributor

It would be good to run the tests on Windows to shake out any development issues with using this library with Windows. Appveyor seems to be the preferred way to do this.
@zombiezen zombiezen added the enhancement New feature or request label Aug 14, 2018
@zombiezen zombiezen added this to the Unplanned milestone Aug 14, 2018
peter-evans added a commit to peter-evans/go-cloud that referenced this issue Oct 24, 2018
@peter-evans
all: add windows ci (google#323)
0c92ccd
@peter-evans
Copy link

Travis now supports windows so I tested the build and there are a number of issues as you can see here:
https://travis-ci.org/peter-evans/go-cloud/jobs/445444398

I'm trying to tackle the cannot find package "example.com/foo" errors. The cause of the error is the forward slash in the package path. I attempted to fix it using filepath.FromSlash which converts to an os-specific separator, but then for some reason it escapes the back slash twice at some later point.

@zombiezen
Copy link
Contributor Author

@peter-evans Cool! Thank you for looking into this! We would really appreciate having the Windows build!

Those are the Wire tests, which run a semi-simulated Go build. The slash in the import path is correct: Go import paths don't change even if OS path separator is different. But in examining the tests, they do translation back and forth between slash-separated and OS-specific paths. It's quite plausible it's incorrect right now, so you would need to examine the data flow to progress.

zombiezen added a commit to zombiezen/go-cloud that referenced this issue Nov 5, 2018
@zombiezen
wire/internal/wire: use on-disk GOPATH in generate tests
37e98df 
The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
neccessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates #78
Updates google#323
@zombiezen zombiezen mentioned this issue Nov 5, 2018
Merged
zombiezen added a commit that referenced this issue Nov 6, 2018
@zombiezen
wire/internal/wire: use on-disk GOPATH in generate tests (#616)
363b278 
The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
necessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates #78
Updates #323
@zombiezen
Copy link
Contributor Author

@peter-evans I believe I have fixed the path separator issue in the Wire tests in #616, but I don't have a Windows machine I can confirm with right now. Please give it another try and see if you still run into issues.

@vangent vangent mentioned this issue Nov 6, 2018
Merged
@vangent
Copy link
Contributor

vangent commented Nov 6, 2018

I send #621 to try enabling the Windows build, and it looks like all tests passed! So I don't think there's anything more to do here.

zombiezen pushed a commit that referenced this issue Nov 6, 2018
@vangent @zombiezen
test: add windows Travis build (#621)
a065595 
Fixes #323
@vangent vangent self-assigned this Nov 6, 2018
@vangent vangent modified the milestones: Unplanned, Sprint 19 Nov 6, 2018
@zombiezen zombiezen mentioned this issue Nov 6, 2018
Merged
@zombiezen
Copy link
Contributor Author

Sadly, it does seem like there is more work: https://travis-ci.com/google/go-cloud/jobs/156718327

Rolling back with #622

@zombiezen zombiezen reopened this Nov 6, 2018
@vangent vangent modified the milestones: Sprint 19, Unplanned Nov 6, 2018
@vangent vangent removed their assignment Nov 6, 2018
@vangent
Copy link
Contributor

vangent commented Nov 6, 2018

@peter-evans feel free to take another look at this!

This was referenced Nov 7, 2018
Merged
Merged
@vangent vangent self-assigned this Nov 7, 2018
This was referenced Nov 8, 2018
Merged
Closed
@vangent vangent added the in progress This is being actively worked on label Nov 8, 2018
@vangent vangent removed this from the Unplanned milestone Nov 8, 2018
@vangent vangent added this to the Sprint 19 milestone Nov 8, 2018
This was referenced Nov 12, 2018
Merged
Merged
@go-cloud-bot go-cloud-bot bot removed the in progress This is being actively worked on label Nov 13, 2018
zombiezen added a commit to google/wire that referenced this issue Nov 28, 2018
@zombiezen
wire/internal/wire: use on-disk GOPATH in generate tests (google/go-c…
64470a2 
…loud#616)

The primary motivation is to permit a move to using go/packages instead
of go/loader. go/packages runs exclusively by shelling out to the go
tool, which precludes use of the in-memory "magic" GOPATH being used
up to this point.

This has a secondary effect of removing a lot of code to support "magic"
GOPATH from the test infrastructure. This is on the whole good, but
necessitated a change in the error scrubbing: since the filenames are
no longer fixed, error scrubbing also must remove the leading
$GOPATH/src lines.

Another related change: since all callers of Generate needed to know the
package path in order to write out wire_gen.go (necessitating a
find-only import search) and Generate already has this information,
Generate now returns this information to the caller. This should further
reduce callers' coupling to Wire's load internals. It also eliminates
code duplication.

This should hopefully shake out any difference in path separators for
running on Windows, but I have not tested that yet.

Updates google/go-cloud#78
Updates google/go-cloud#323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vangent vangent

Labels
enhancement New feature or request
Projects
None yet
Milestone
Sprint 19
Development

No branches or pull requests
3 participants
@zombiezen @peter-evans @vangent