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

Version strings are still wrong #8571

Closed
lnicola opened this issue Apr 19, 2021 · 25 comments · Fixed by #8643
Closed

Version strings are still wrong #8571

lnicola opened this issue Apr 19, 2021 · 25 comments · Fixed by #8643
Assignees
Labels
Broken Window Bugs / technical debt to be addressed immediately S-actionable Someone could pick this issue up and work on it right now

Comments

@lnicola
Copy link
Member

lnicola commented Apr 19, 2021

We use git describe to get a version string, but the release is tagged only after uploading the artifacts to GitHub. This means that we make a version string using the previous release name.

One way to fix this would be to force a certain version string in the workflow for non-nightly releases, but it feels a bit hacky.

@lnicola lnicola added S-actionable Someone could pick this issue up and work on it right now Broken Window Bugs / technical debt to be addressed immediately labels Apr 19, 2021
@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 19, 2021

Nightly version strings look rather odd too:

Nightly version string for 18th of April:
rust-analyzer version: 2021-04-12-83-g19fc1f333
Nightly version string for 19th of April:
rust-analyzer version: 2021-04-12-92-g7570212a5

As a user, I find those a bit counter-intuitive:

  • the 2021-04-12 date is "old" from nightly's perspective and does not help to identify when exactly the nightly was created
  • no idea what 83 and 92 parts are
  • the last parts (g19fc1f333 and g7570212a5) look like hashes, but I could not find them or their parts among the commits

I would preferred a nightly version string that could instantly show when this nightly was created and what code was used for that, for instance: 2021-04-19-d39873e where 2021-04-19 is a date of a nightly build and d39873e is the last commit on the moment of the build.
Additionally, some nightly prefix/suffix could also be used to make it easily distinguishable from the non-nightly version strings.

@lnicola
Copy link
Member Author

lnicola commented Apr 19, 2021

83 and 92 are the number of commits between the last tag and the current commit. So between 18th and 19th we had 9 new commits (some of them merges, of course). This is the behavior of git describe and also common in hg IIRC.

The g-hash is also a common convention, you need to skip the g if you want to find the current commit. See e.g. https://archlinux.org/packages/extra/x86_64/mutter/ at version mutter 40.0+55+gf4f82bcb9-1 in my Linux distro (not sure how they get + instead of - though).

Additionally, some nightly prefix/suffix could also be used to make it easily distinguishable from the non-nightly version strings.

The presence of the -NUM-g-hash part normally indicates an untagged release, with the exception of this bug.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 19, 2021

83 and 92 are the number of commits between the last tag and the current commit.

That also feels odd, since we're releasing nightly every day and while we've been trying to release some extra releases recently, 8 extra builds feels odd.
Also, since we're releasing new tags on a weekly basis, 83 nightly builds since the last week's release is way too much?
As a personal opinion, relative numbers bring not that much sense for nightly releases and also hard to calculate compared to an exact date + hash.

you need to skip the g

I've noted that g is an extra addition, but I cannot locate a substring of that has too, like f333: it's just not in the recent master commits: https://github.com/rust-analyzer/rust-analyzer/commits/master

part normally indicates an untagged release

Yes, my point is to strongly distinguish nightlies from other untagged releases such as local builds, to eliminate any possible confusion. But that's least of my concerns related to version strings.

@lnicola
Copy link
Member Author

lnicola commented Apr 19, 2021

That also feels odd, since we're releasing nightly every day and while we've been trying to release some extra releases recently, 8 extra builds feels odd.

The extra release wasn't actually tagged or released, so it doesn't count. In that example there are 9 commits on the 18th, which sounds reasonable. I'm not sure what the 8 you mention is.

Also, since we're releasing new tags on a weekly basis, 83 nightly builds since the last week's release is way too much?

They are commits, not builds.

I've noted that g is an extra addition, but I cannot locate a substring of that has too, like f333

Why f333? You need to skip the g and take the rest:

$ git show --oneline 19fc1f333 7570212a5
19fc1f333 Merge #8559
7570212a5 (HEAD -> release, tag: nightly, tag: 2021-04-19, upstream/release) Merge #8569

Yes, my point is to strongly distinguish nightlies from other untagged releases such as local builds, to eliminate any possible confusion.

I think we can pass --dirty for that: 2021-04-19-6-gdc7b74ffd-dirty. Or we can put in -nightly for nightlies to know it's an official release, since the user might build from the same SHA, but with a different toolchain or something.

@SomeoneToIgnore
Copy link
Contributor

I'm not sure what the 8 you mention is.

Oh, never mind me, I have to wake up apparently: I've thought about the # of builds, not # of commits even after reading your comment, sorry.
Yet still not sure why exactly we care about relative values here, but not insisting on anything.

Why f333?

I'm trying to act as a r-a user that usually don't have any r-a code checked out (and I myself often prefer GitHub over git commands due to browsing things from a mobile device, for instance).
So I don't have a code, but have GitHub and I want to check which commits does this nightly contain, so I open the commits and look for hashes, just searching them in the web ui, where f333 is enough: I look for a substring via the browser after all.

Yet GitHub link I've posted above does not show that commit for some reason ergo the confusion.

@lnicola
Copy link
Member Author

lnicola commented Apr 19, 2021

So I don't have a code, but have GitHub and I want to check which commits does this nightly contain, so I open the commits and look for hashes, just searching them in the web ui, where f333 is enough: I look for a substring via the browser after all.

That's a strange workflow, I usually copy the hash prefix and put it in the URL like 19fc1f333.

It doesn't show up for you because git describe uses 9 hex digits, but GitHub only displays 7, so that one shows up as 19fc1f3. It's better to start typing them from the beginning.

@SomeoneToIgnore
Copy link
Contributor

Thank you for the explanations, now I feel even more flattened 😬

Feels like I was wrong from the start and bring no extra sense with these comments, so clean them if you want to, while I'm trying to adjust to a different workflow.

@lnicola
Copy link
Member Author

lnicola commented Apr 19, 2021

No worries, it's Monday. It was good feedback, but I think using the standard git describe output is reasonable since (some) people will recognize it. We should at least add a -nightly suffix, though.

@flodiebold
Copy link
Member

Can we create the tag locally before building the release? I didn't find in the release workflow where the tagging actually happens.

@lnicola
Copy link
Member Author

lnicola commented Apr 19, 2021

The tagging is done by the github-release workflow. We can make the tag before (I don't know if it causes any problems), but we might as well pass an override as an env variable to xtask dist -- there's not much that potential to have them get out of sync.

@matklad
Copy link
Member

matklad commented Apr 19, 2021

Hm.... I think "date", "commit hash" and "release" channel are just dissimilar bits of meta, and that it might not be best to try to cajole git to give us them all.

How about we do this?

  • use git rev-parse --short HEAD for commit hash (unambiguous)
  • use date --iso for build date (this is mildly confusing due to timezones and nightlyness, but we'd have to live with that. I guess, we can shift releases to be at one rather that at UTC midnight to protect from the clock drift somewhat)
  • use env variable / xtask dist flag to add dev / nightly / `` for various builds

@matklad
Copy link
Member

matklad commented Apr 19, 2021

So, I imagine a string like this will be suitable for the users:

rust-analyzer g75371eb0f 2021-04-12 dev

I am also wondering, should be have rust-analyzer --version --json?

{
  "commit": "g75371eb0f",
  "buildDate": "2021-04-12",
  "channel": "dev",
}

I feel that some folks might be parsing this output, so having a stable breaking JSON version seems good?

EDIT:

a stable breaking JSON version

Clearly, I am one more person in the thread who needs a wake-up pu'er

@eminence
Copy link
Contributor

While we're on the subject of version numbers, I'd like to bump rust-analyzer/rust-analyzer.github.io#56 for attention if possible

@SomeoneToIgnore
Copy link
Contributor

So I have a nightly plugin version, nightly checkbox in settings toggled and
rust-analyzer version: 617535393 stable as the version output:

image

I don't see a date and see stable instead of nightly, so I suggest reopening the issue.
(and somebody else than me using nightly, preferably, to increase the nightly-related issue discoverability)

@Veykril
Copy link
Member

Veykril commented Apr 26, 2021

I do get a date on newest nightly, but still with the stable tag Code_Gpxi2HTMv6

Edit: This is on windows

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 26, 2021

Ok, so more details then: I'm using macOs, so presumably something during its binary build process also interferes with the dates?

@matklad
Copy link
Member

matklad commented Apr 26, 2021

well, at least I got linux right :D

@matklad
Copy link
Member

matklad commented Apr 26, 2021

#8666 should fix it

@Veykril
Copy link
Member

Veykril commented Apr 26, 2021

Looks like it works for me now
Code_kqOWnooATU

@SomeoneToIgnore
Copy link
Contributor

image

It's definitely better for me, but.

@matklad
Copy link
Member

matklad commented Apr 26, 2021

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 26, 2021

$ date --utc +%Y-%m-%d
date: illegal option -- -
usage: date [-jnRu] [-d dst] [-r seconds] [-t west] [-v[+|-]val[ymwdHMS]] ...
            [-f fmt date | [[[mm]dd]HH]MM[[cc]yy][.ss]] [+format]

$ echo $?
1

$ which date
/bin/date

the only thing resembling UTC I could locate in man was this:

ENVIRONMENT
     The following environment variables affect the execution of date:

     TZ      The timezone to use when displaying dates.  The normal format is a pathname relative to /usr/share/zoneinfo.  For example, the command ``TZ=America/Los_Angeles date''
             displays the current time in California.  See environ(7) for more information.

mac has pretty weird binutils indeed, but weird that we don't fail on such cases.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Apr 26, 2021

$ TZ=UTC date +%Y-%m-%d
2021-04-26

$ TZ=etc/GMT+122 date +%Y-%m-%d
2021-04-21

$ TZ=SOMETHING_INSANE date +%Y-%m-%d
2021-04-26

It treats this parameter quite liberally though, so I cannot be sure it understands my TZ=UTC correctly, despite the

$ cat /usr/share/zoneinfo/UTC
TZif2UTCTZif2UTC
UTC0

presence

@SomeoneToIgnore
Copy link
Contributor

Found it:

     -u      Display or set the date in UTC (Coordinated Universal) time.
$ date -u +%Y-%m-%d
2021-04-26

bors bot added a commit that referenced this issue Apr 26, 2021
8668: Use more cross-platform utc `date` argument r=matklad a=SomeoneToIgnore

Part of #8571

```
$ docker run -it --rm ubuntu:20.04 bash
root@7393d1e7bbad:/# date -u +%Y-%m-%d
2021-04-26
```

```
$ date -u +%Y-%m-%d
2021-04-26

$ uname -a
Darwin alaptop.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64
```

Some of the places where I've change this do not really require it (since macos bin would have failed with `--iso` param also), but I've changed them for consistency.

Co-authored-by: Kirill Bulatov <mail4score@gmail.com>
@SomeoneToIgnore
Copy link
Contributor

🎉 mac works now, thank you.
Windows seems to be working too and Linux wasn't even broken, so closing this.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Broken Window Bugs / technical debt to be addressed immediately S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants