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.

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

Use sha256 hash of extra coordinates for cache dir name, pepify #62

Merged
merged 2 commits into from
Aug 3, 2021

Conversation

kephale
Copy link

@kephale kephale commented May 24, 2021

To address #31 (comment)

@hanslovsky
Copy link
Member

Can you push without formatting changes? That will make it easier to review the actual changes.

@ctrueden
Copy link
Member

ctrueden commented May 25, 2021

I'm a fan of the pepification. I agree with @hanslovsky that doing it as a separate commit would be best.

Here is how I typically tease such things apart using git:

git reset HEAD^ # undo the commit
git stash
do-the-pepification # so the unchanged code gets pepified
git commit -a -m 'Pepify!'
git checkout 'stash^{tree}' -- . && git stash drop # force exactly what you stashed back into the working copy
git commit -a -m 'My awesome change, clean this time'

Alternately, @kephale, if you don't have time... just tell us the command to do the pepification? Then I can do the above process myself.

@kephale
Copy link
Author

kephale commented May 25, 2021

Okey doke, @hanslovsky @ctrueden all done.

@ctrueden thank you for the commands! That was super useful

@kephale
Copy link
Author

kephale commented May 25, 2021

FWIW, the pepify is with https://github.com/google/yapf which is attached to the python mode in my emacs

@hanslovsky
Copy link
Member

Thanks @kephale
I have a pretty busy schedule right now but will try to make some time this weekend to look over it.

@kephale
Copy link
Author

kephale commented May 26, 2021

Sure, no rush. It is just 4 lines, but I'm pip installing the branch now anyway, so this doesn't block me anymore.

@hanslovsky
Copy link
Member

Can you share an example path in the ~/.jgo cache? I would expect something like

~/.jgo/${groupId}/${artifactId}/${version}/e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855

Is that correct?

As a user, can I see what the coordinate is for a workspace/hash? I think it would make sense to add a file to the workspace that shows coordinate or coordinate[1:]?
This file may also be helpful in the (extremely unlikely) case of a hash collision - jgo could throw an error instead of executing the wrong Java application.

@kephale
Copy link
Author

kephale commented May 29, 2021

.jgo/sc.fiji/bigdataviewer-vistools/1.0.0-beta-29-SNAPSHOT+eba447da3454eafae1d5fd4a362d3457b84d0c5a20db7e40767c390d1dab6cba

This solved my use case, and I was able to move on to the work it blocked. Saving the coordinate to file makes sense, will put it on the TODO.

@kephale kephale force-pushed the hash-cache branch 4 times, most recently from ff4e216 to 4abdcc7 Compare June 1, 2021 08:07
@kephale
Copy link
Author

kephale commented Jun 1, 2021

@hanslovsky okey doke. I adjusted the path and added the coordinates file as well:

$ cat ~/.jgo/net.imagej/imagej/2.1.0/ad412f409ae00c86c7c3f165d0d787ed4dd7a42191903e4f0256bebee4b4fa45/coordinates.txt 
net.imagej:imagej:2.1.0
graphics.scenery:scenery:937ba10
org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.20
org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9
sc.fiji:bigdataviewer-core:10.1.1-SNAPSHOT
sc.iview:sciview:f4dd286

Copy link
Member

@hanslovsky hanslovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, will leave it to @ctrueden to merge it.

@kephale
Copy link
Author

kephale commented Jun 22, 2021

bump

1 similar comment
@kephale
Copy link
Author

kephale commented Jul 9, 2021

bump

@cthoyt
Copy link
Contributor

cthoyt commented Aug 2, 2021

I would suggest using black to deterministically reformat the whole package. I could send a PR for that

Update: see #63

Kyle Harrington added 2 commits August 2, 2021 19:06
Workspace dir is now ~/.jgo/${groupId}/${artifactId}/${version}/hash
Coordinates are written into workspace as coordinates.txt
@ctrueden ctrueden merged commit 7d16167 into master Aug 3, 2021
@ctrueden ctrueden deleted the hash-cache branch August 3, 2021 00:08
@cthoyt
Copy link
Contributor

cthoyt commented Aug 3, 2021

@ctrueden now that this is merged, do you think you can mint a new release to PyPI?

@ctrueden
Copy link
Member

ctrueden commented Aug 3, 2021

do you think you can mint a new release to PyPI?

@cthoyt I pushed jgo 1.0.2 to PyPI. Please let me know how it works for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants