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

cli version: Show date as part of the version next to the commit id, shorten commit id #2034

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Aug 11, 2023

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (src/config-schema.json)
  • I have added tests to cover my changes

@ilyagr ilyagr marked this pull request as ready for review August 11, 2023 01:28
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 11, 2023

We discussed the Nix part with @Ralith. He suggested

preConfigure = ''
    NIX_JJ_GIT_HASH=$(git <something> --git-dir=${./.}/jj/repo/store/git)
'';

I might look into that, or I could leave it for some more Nix person to do.

@thoughtpolice
Copy link
Collaborator

thoughtpolice commented Aug 11, 2023

Something like this should work, I think, in flake.nix, but we should change the date format from git log to be YYYYMMDD to be consistent:

gitRev = "${builtins.substring 0 8 (self.lastModifiedDate or self.lastModified or "19700101")}|${self.shortRev or "dirty"}";

@thoughtpolice thoughtpolice mentioned this pull request Aug 11, 2023
4 tasks
@thoughtpolice
Copy link
Collaborator

Commit cf39ada on branch push-numyszkprmty contains the changes I think you need. Here's an example of what this looks like when using the interactive shell, basically the format <YYYYMMDD>|<HASH>{-dirty}

austin@GANON:~/src/jj$ echo $NIX_JJ_GIT_HASH 
20230811|cf39adafac5ed1f5adc112ede325d6fcbfd7d2b7
austin@GANON:~/src/jj$ echo $NIX_JJ_GIT_HASH 
20230811|17b45d642f39351500d74bc07fe1d70ee0f7bb5c-dirty

The first is a clean commit and the second is dirty. This uses the dirtyRev and dirtyShortRev features from Nix that are only available in Nix as of a month or so ago, so it's very recent; on older versions of Nix, for backwards compatibility the version in a dirty repository will look like this:

austin@GANON:~/src/jj$ echo $NIX_JJ_GIT_HASH 
20230811|dirty

Which isn't useful, but mostly only happens to us during development, so I think it's OK.

If you can unify these two version paths to be the same, that would be great. Feel free to mold this commit to your needs.

@Ralith
Copy link
Collaborator

Ralith commented Aug 11, 2023

self.lastModifiedDate or self.lastModified

Won't this be different than the commit date?

@thoughtpolice
Copy link
Collaborator

lastModifiedDate is the commit time of the rev in question. (I think it would have to be, otherwise it would break determinism.)

@necauqua
Copy link
Collaborator

The point of NIX_JJ_GIT_HASH was that nix cleans up the .git folder and build.rs is not run in the jj/git repo, so it couldn't load the hash.

I now realised that a better approach would probably be to set NIX_GIT_REPO="${./.}/jj/repo/store/git" or something similar, and have --git-dir set to that in build.rs - that way there is no two separate ways (in nix and in rust) to calculate this same bleeding version value

@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 11, 2023

@thoughtpolice Thanks! I don't feel like adding that to this PR right now, but it looks like it'd be easy to add. Either the PR can wait or it can go into a separate one.

I think adding "-dirty" should be a separate change, as it'd require adjusting a test, and ideally it'd go together with supporting it in the regular Rust code.

I now realised that a better approach would probably be to set NIX_GIT_REPO="${./.}/jj/repo/store/git" or something similar,

This sounds fine to me, but I don't grok Nix philosophy or implementation enough to really know whether this is a good idea. I think it should be a separate discussion/PR in any case.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 11, 2023

Also, I'll leave it up to (the plural) you to decide which Nix expression for the date is exactly right. It would be nice to be consistent, but we can always fix it later.

@ilyagr ilyagr force-pushed the verdate branch 3 times, most recently from 7c5ab8e to 383ab92 Compare August 12, 2023 03:48
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 12, 2023

I changed the date format to YYYYMMDD and incorporated some of @thoughtpolice's changes. Thank you, they were very helpful!

I rewrote them a bit and removed the portion having to do with dirty revisions for now. We should add it back when the rest of build.rs understands dirty revisions.

@ilyagr ilyagr force-pushed the verdate branch 3 times, most recently from ed4087c to 934caa2 Compare August 12, 2023 04:06
flake.nix Outdated Show resolved Hide resolved
@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 12, 2023

There's a mild inconsistency in dates. I'm guessing one is in UTC and one is in the local timezone. I think that's fine, resolution of plus/minus a day should be enough here.

image

However, if somebody wants to ensure UTC everywhere it'd certainly be nicer! The UTC timestamp feature suggested in https://discord.com/channels/968932220549103686/969829516539228222/1139656152926404649 would be helpful here.

@thoughtpolice
Copy link
Collaborator

thoughtpolice commented Aug 14, 2023

FWIW, if you can set TZ=UTC0 in the environment on the invocation to jj, it might handle that fine on both Windows and Unix? It's POSIX specified, at least. This works on my Windows machine, for example:

PS C:\code\thingy> git show --quiet --date=local --format="%cd"
Tue Apr 16 02:17:33 2019
PS C:\code\thingy> $env:TZ = "UTC0"; git show --quiet --date=local --format="%cd"
Tue Apr 16 07:17:33 2019
PS C:\code\thingy> $env:TZ = "UTC0"; git show --quiet --date='format-local:%Y%m%dT%H%M%SZ' --format="%cd"
20190416T071733Z

@ilyagr
Copy link
Collaborator Author

ilyagr commented Aug 15, 2023

FWIW, if you can set TZ=UTC0 in the environment on the invocation to jj, it might handle that fine on both Windows and Unix? It's POSIX specified, at least.

Thanks, that sounds good. I don't think I'll use it in this commit, though. It's blocked on the jj side by #2080, I think.

@ilyagr ilyagr force-pushed the verdate branch 6 times, most recently from 635b24c to 136a89b Compare August 21, 2023 06:39
@ilyagr ilyagr marked this pull request as draft August 22, 2023 04:03
@ilyagr ilyagr force-pushed the verdate branch 4 times, most recently from 5d2f17b to f2f51c4 Compare April 5, 2024 23:58
@ilyagr ilyagr force-pushed the verdate branch 3 times, most recently from 9787a13 to c3a07de Compare April 17, 2024 00:38
@ilyagr ilyagr force-pushed the verdate branch 4 times, most recently from 2bed6cf to a5d36f9 Compare May 9, 2024 04:31
@ilyagr ilyagr force-pushed the verdate branch 2 times, most recently from f41cdaa to 94ad7b5 Compare May 20, 2024 22:45
@ilyagr ilyagr force-pushed the verdate branch 4 times, most recently from 7df5e36 to dd1af70 Compare May 27, 2024 18:12
@ilyagr ilyagr marked this pull request as draft June 1, 2024 04:27
@ilyagr ilyagr force-pushed the verdate branch 4 times, most recently from db5e582 to fa228d5 Compare June 16, 2024 03:34
ilyagr added 4 commits June 19, 2024 15:10
The date comes from the commiter date of the commit. This is so that
it's easy to tell at a glance how old a version is.

To make it easier to get UTC date, we first obtain it as a Unix
timestamp, which should always be UTC. This will make the
interaction with Nix in a subsequent commit easier.
16 characters ought to be enough for anyone :)
Thanks to @thoughtpolice for providing the original version of the nix code; I
edited it a bit.
Replace `jj 0.10.0-20231027-bb8af5adc4f98ff2` with `jj 0.10.0+20231027-bb8af5adc4f98ff2`.

According to https://semver.org, the build metadata should be separated by a
`+`, not `-`. If semver rules are pedantically followed, `jj
0.10.0-20231027-bb8af5adc4f98ff2` sorts before `jj 0.10.0`, whereas the build
metadata is ignored for the purposes of ordering.

I'm not sure whether anybody else is likely to treat the full `jj version`
string as a semver this pendantically, but we can :).

I discovered this when thinking about jj-vcs#2258; see that PR for a few more details
about pedantic semvers.
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.

5 participants