Masterminds / glide

Package Management for Golang
https://glide.sh
Other
8.15k stars 541 forks source link

Glide on Windows writes the lock file with incorrect slashes #235

Closed colemickens closed 8 years ago

colemickens commented 8 years ago

Deduplication and identification of included packages seems to be affected as well.

Here's the diff of what glide did when running glide up on windows:

diff --git a/glide.lock b/glide.lock
index 03c385f..a14e606 100644
--- a/glide.lock
+++ b/glide.lock
@@ -1,19 +1,47 @@
 hash: 46cd6d2d3f2840288fad453b3d7d8309d53ed56f84794a6edba7c71968b95142
-updated: 2016-01-28T01:15:34.905936244-08:00
+updated: 2016-01-29T12:20:43.7050665-08:00
 imports:
 - name: github.com/Azure/go-autorest
-  version: 20a00a4f1f2a2abdfb98e6beb1126f1d36d7063a
+  version: 8ced4882956df3128b6d81a183d8e0757ac825b3
   subpackages:
   - /autorest
   - autorest/azure
   - autorest/date
   - autorest/to
 - name: github.com/dgrijalva/jwt-go
-  version: f164e17f59b82642a3895ba065c385db6c547344
+  version: afef698c326bfd906b11659432544e5aae441d44
+- name: github.com\Azure\go-autorest
+  version: 8ced4882956df3128b6d81a183d8e0757ac825b3
+- name: github.com\dgrijalva\jwt-go
+  version: afef698c326bfd906b11659432544e5aae441d44
 - name: golang.org/x/crypto
-  version: 346896d57731cb5670b36c6178fc5519f3225980
+  version: 1f22c0103821b9390939b6776727195525381532
   subpackages:
   - /pkcs12
+- name: golang.org\x\crypto
+  version: 1f22c0103821b9390939b6776727195525381532
+- name: golang.org\x\crypto\blowfish
+  version: ""
+- name: golang.org\x\crypto\nacl\secretbox
+  version: ""
+- name: golang.org\x\crypto\openpgp\armor
+  version: ""
+- name: golang.org\x\crypto\openpgp\errors
+  version: ""
+- name: golang.org\x\crypto\openpgp\packet
+  version: ""
+- name: golang.org\x\crypto\openpgp\s2k
+  version: ""
+- name: golang.org\x\crypto\pkcs12
+  version: ""
+- name: golang.org\x\crypto\pkcs12\internal\rc2
+  version: ""
+- name: golang.org\x\crypto\poly1305
+  version: ""
+- name: golang.org\x\crypto\salsa20\salsa
+  version: ""
 - name: gopkg.in/check.v1
-  version: 11d3bc7aa68e238947792f30573146a3231fc0f1
+  version: 4f90aeace3a26ad7021961c297b22c42160c7b25
+- name: gopkg.in\check.v1
+  version: 4f90aeace3a26ad7021961c297b22c42160c7b25
 devImports: []

Note that go-autorest is re-added to the lock file, even though it's already there.

Here's the diff it should have created: (this was generated from glide up on a Linux box)

diff --git a/glide.lock b/glide.lock
index 58c0c35..03c385f 100644
--- a/glide.lock
+++ b/glide.lock
@@ -1,21 +1,19 @@
 hash: 46cd6d2d3f2840288fad453b3d7d8309d53ed56f84794a6edba7c71968b95142
-updated: 2016-01-29T13:16:26.939463551-08:00
+updated: 2016-01-28T01:15:34.905936244-08:00
 imports:
 - name: github.com/Azure/go-autorest
-  version: 8ced4882956df3128b6d81a183d8e0757ac825b3
+  version: 20a00a4f1f2a2abdfb98e6beb1126f1d36d7063a
   subpackages:
   - /autorest
   - autorest/azure
   - autorest/date
   - autorest/to
 - name: github.com/dgrijalva/jwt-go
-  version: afef698c326bfd906b11659432544e5aae441d44
-  subpackages:
-  - .
+  version: f164e17f59b82642a3895ba065c385db6c547344
 - name: golang.org/x/crypto
-  version: 1f22c0103821b9390939b6776727195525381532
+  version: 346896d57731cb5670b36c6178fc5519f3225980
   subpackages:
   - /pkcs12
 - name: gopkg.in/check.v1
-  version: 4f90aeace3a26ad7021961c297b22c42160c7b25
+  version: 11d3bc7aa68e238947792f30573146a3231fc0f1
 devImports: []
mattfarina commented 8 years ago

Thanks for providing this bug. I know what's going on. Working with the filesystem in a cross platform manner can cause bugs like this to pop up. I'll fix it shortly.

mattfarina commented 8 years ago

@colemickens Can you check the latest on master. I think I fixed this.

colemickens commented 8 years ago

@garimakhulbe Can you try this on your Windows machine and report back the diff between glide.lock before and after running glide up ?

colemickens commented 8 years ago

@mattfarina We confirmed that we were on the very latest of glide when that was generated. It seems to not be resolved.

mattfarina commented 8 years ago

Thanks for your patience. I'm now setup to work on this in my Windows environment and I'll test it against the Azure SDK for Go.

mattfarina commented 8 years ago

@colemickens @garimakhulbe can you take a look at the latest on master. I have it working in my Windows environment now.

colemickens commented 8 years ago

CC @boumenot

colemickens commented 8 years ago

@mattfarina It looks like there might be one remaining issue, since it's trying to add the unused subpackages in Windows still.

mattfarina commented 8 years ago

@colemickens I'll take a look at that.

mattfarina commented 8 years ago

The latest on master is not venerating the right subpackages for me. Can you try again.

colemickens commented 8 years ago

@mattfarina Sorry, I don't follow, are you saying there's a problem with azure-sdk-for-go master?

mattfarina commented 8 years ago

@colemickens Well, that typo of mine messes with the meaning. Sorry about that. The latest on master is now generating the right subpackages for me. Can you please try again?

colemickens commented 8 years ago

@garimakhulbe Can you give it a shot again? I still don't have a Windows machine setup with the relevant stuff.

colemickens commented 8 years ago

I still see two issues:

mattfarina commented 8 years ago

This latest commit should fix the issue where the same package is present with and without the leading /.

The pkcs12 package imports the pkcs12/internal/rc2 package which is why it shows up in the lock file. It's imported in the pkcs12/crypto.go file. This is the complete tree of referenced packages which is why it's present in the lock.

Is there anything else here that needs fixing?

colemickens commented 8 years ago

I'm just not sure why rc2 was only added in Windows. Transitive dependencies are normally shown in the lock file, not the yaml file, in my experience. On Feb 9, 2016 11:04 PM, "Garima Khulbe" notifications@github.com wrote:

[image: image] https://cloud.githubusercontent.com/assets/9362976/12938929/a3451a1e-cf6e-11e5-93fe-91764bfa04f2.png

LGTM. @colemickens https://github.com/colemickens, please confirm.

— Reply to this email directly or view it on GitHub https://github.com/Masterminds/glide/issues/235#issuecomment-182200973.

colemickens commented 8 years ago

Maybe the exception is when the transitive dependency is a subpackage or from the same repo? But it doesn't get added in Linux (that was about two weeks ago though). On Feb 9, 2016 11:09 PM, "Cole Mickens" cole.mickens@gmail.com wrote:

I'm just not sure why rc2 was only added in Windows. Transitive dependencies are normally shown in the lock file, not the yaml file, in my experience. On Feb 9, 2016 11:04 PM, "Garima Khulbe" notifications@github.com wrote:

[image: image] https://cloud.githubusercontent.com/assets/9362976/12938929/a3451a1e-cf6e-11e5-93fe-91764bfa04f2.png

LGTM. @colemickens https://github.com/colemickens, please confirm.

— Reply to this email directly or view it on GitHub https://github.com/Masterminds/glide/issues/235#issuecomment-182200973.

colemickens commented 8 years ago

We can safely ignore my first comment from a few hours ago since we are in fact looking at the lock file in the diff.

I updated glide on my Linux box. It is now producing the same diff as what @garimakhulbe has shown above.

However, rc2 has always been required by pkcs12... so it looks like we were actually seeing another bug here that has been fixed in the last few weeks. Does that sound accurate @mattfarina? If so, we can certainly close this.

Thanks for being so responsive.

mattfarina commented 8 years ago

I think we can close this one, then. If there are more issues please let us know.