-
Notifications
You must be signed in to change notification settings - Fork 1k
Chase improvements to input hashing in gps #97
Conversation
Ugh...failure seems to probably be this: Which is probably caused by this change I merged: sdboyer/gps#141 . Guess that has unintended consequences, and I need to roll it back... |
Adds a Obviously this is hacky, I'm happy to refactor if folks find it too objectionable. |
hm, problem isn't sdboyer/gps#141. gotta be something else. |
ah, i just needed to update the |
If i run
|
ugh. probably a stale repo on disk. i thought i had these cases covered, but dear god so much code. could you once you do that, go ahead and blow away (not that that's an acceptable solution, just trying to work out why it's happening) |
i think its because this PR updated vendor and not the lock & manifest tho right |
oh nevermind this does change the lock... ok weird |
|
ok after i blew it away I got no change to hash \0/ |
OK i'll rebase and fix this sometime tonight. i'm gonna need a serious regression test for this in gps, too. i thought i had this covered, but it's annoying enough to test for that i never actually set one up. now that i see i clearly don't have it covered, there's no more delaying it. |
@@ -0,0 +1,53 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is missing the license header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i fixed
Signed-off-by: Jess Frazelle <[email protected]>
"github.com/sdboyer/gps" | ||
) | ||
|
||
func (cmd *hashinCommand) Name() string { return "hash-inputs" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait why do we need this command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclusively for debugging. i'm totally fine to pull it out if it seems like overkill to add it
@@ -37,5 +37,5 @@ func (a analyzer) DeriveManifestAndLock(path string, n gps.ProjectRoot) (gps.Man | |||
|
|||
func (a analyzer) Info() (name string, version *semver.Version) { | |||
v, _ := semver.NewVersion("v0.0.1") | |||
return "example-analyzer", v | |||
return "hoard", v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nest
func (cmd *hashinCommand) Args() string { return "" } | ||
func (cmd *hashinCommand) ShortHelp() string { return "" } | ||
func (cmd *hashinCommand) LongHelp() string { return "" } | ||
func (cmd *hashinCommand) Hidden() bool { return false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we do need it, it should be hidden right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL literally the reason why i added the Hidden
func and, in my copy/paste fervor, i forgot to use it correctly here
The only real change here is improvements to input hashing. That's both algorithm changes:
foo
and a constraint was on a tag/versionfoo
. (This is visible in the output, where you'll see things likesvc-^1.0.0
when the input constraint was^1.0.0
.)And some output changes, via the
HashingInputsAsString()
func:Because this fixes (let's hope) issues with memoization, I also removed
wipeMemo()
, so this fixes #59.