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

Attach commit as package.json#gitHead in prerelease versions #47932

Merged
merged 4 commits into from
Mar 7, 2023

Conversation

Andarist
Copy link
Contributor

:warn: I didn't do any extensive testing - this PR should be verified by somebody who actually understands how this script is used and how this might affect other things.

Why?

I've recognized a regression between nightly builds of TS (well, when you were releasing nightlies for 3.8). I have the exact version in which the valid error has stopped being reported by TS but I cannot easily check from which commit this build was prepared. This helps with that.

Note that I don't care where this commit would be written - it could be in a root .json, in the README, or somewhere else. It's just that per semver spec attaching such metadata in the version is allowed and this shouldn't affect any semver-aware algorithms for matching versions, dependency ranges etc. This is described here: https://semver.org/#spec-item-10

While this is a very little-known "feature", I've seen this being used "in the wild".

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Feb 17, 2022
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@MartinJohns
Copy link
Contributor

MartinJohns commented Feb 17, 2022

#36960

Don't know if it's still the same way, but:

We deploy in a separate vm from one with access to the git repo for security - is there a way to set this commit without fs git access?

@minestarks
Copy link
Member

@DanielRosenwasser I have no opinion on the change itself, but this would be a breaking change for the pipeline that syncs versions across to the ADO repo, so we'll have to make a corresponding change. Let's just make sure that's handled before we merge.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 4, 2022

Note - I don't mind using some other solution for this, I only care about the commit being available somewhere in the published artifact. It made sense for me to include it here as it is some well-defined sport where such a thing can be put, any other place I would have to "figure out" by myself/pick at random and I didn't know what would be the preference of the team.

In a sense, this PR is more of a conversation starter than something that has to land ASAP.

@DanielRosenwasser
Copy link
Member

I think that my main concern is that if the commit metadata lands in the package version, it becomes harder (maybe not easily possible?) to get a version of TypeScript on a specific date, which is often how we offhandedly bisect. For example, I'll try installing [email protected] to get the most recent version, but I don't think I could write that without knowing the exact commit that was published on that nightly.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 5, 2022

That's actually not true - this build metadata is completely ignored when it comes to matching semver ranges etc. You could install this just fine.

@DanielRosenwasser
Copy link
Member

Could you elaborate? If I try installing a partial date suffix, it will fail.

> npm install [email protected]
npm WARN old lockfile
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile
npm ERR! code ETARGET
npm ERR! notarget No matching version found for [email protected].
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

npm ERR! A complete log of this run can be found in:
npm ERR!     [UserPath]\AppData\Local\npm-cache\_logs\2022-03-06T22_16_28_756Z-debug.log

If I give a full published version (e.g. [email protected]), it seems to succeed.

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 7, 2022

@DanielRosenwasser The build metadata is not part of the actual version number. It makes no difference whether you install [email protected] or [email protected]+myfancybuildmetadata. See the relevant spec item of SemVer: https://semver.org/#spec-item-10

Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence.

@Andarist
Copy link
Contributor Author

Andarist commented Mar 7, 2022

Yea, and actually - that build metadata is completely ignored when installing. This particular package and version doesn't have any build metadata and it gets installed successfully:

yarn add [email protected]+somerandomstaff

We can also test a package (@react-aria/utils) that has some build metadata in its version field set to 3.0.0-nightly.1460+2c25922e2 in one of its nightlies~:

yarn add @react-aria/[email protected]+usecompletelydifferentmeta

This one installs just fine as well.

@orta
Copy link
Contributor

orta commented Mar 9, 2022

I agree with all the build metadata / semver discussion, but it might just be simpler to put this on a new field in the package json

@Andarist
Copy link
Contributor Author

Andarist commented Mar 9, 2022

What do you think about package.json#gitHead? This is a field added by some tools (like Lerna) when packing packages.

@Andarist Andarist changed the title Attach build metadata (commit) to the generated prerelease version Attach commit as package.json#gitHead in prerelease versions Mar 6, 2023
@Andarist
Copy link
Contributor Author

Andarist commented Mar 6, 2023

switched this one to use package.json#gitHead, cc @jakebailey

@jakebailey
Copy link
Member

Many thanks!

@jakebailey jakebailey merged commit 9f8a160 into microsoft:main Mar 7, 2023
@jakebailey
Copy link
Member

Okay, what? How did this pass CI?

npx hereby scripts             
Using ~/work/TypeScript/Herebyfile.mjs to run scripts
Starting scripts
scripts/configurePrerelease.mjs:55:22 - error TS2339: Property 'gitHead' does not exist on type 'PackageJson'.

55     packageJsonValue.gitHead = execFileSync("git", ["rev-parse", "HEAD"], { encoding: "utf8" }).trim();
                        ~~~~~~~


Found 1 error.

Error in scripts in 3.2s
Error: Process exited with code: 1
Completed scripts with errors in 3.2s

We test this in CI so I don't know how this happened...

@Andarist
Copy link
Contributor Author

Andarist commented Mar 7, 2023

This property displays as any in my IDE, I assumed that PackageJson type has an index signature ;p

@jakebailey
Copy link
Member

Right, but it's only any because it's actually an error... The type is defined at the top of the file. I don't get what's happening here. It's failing over on #53134 now, but not in main?

@Andarist
Copy link
Contributor Author

Andarist commented Mar 7, 2023

Note that I dont see an error at this location in the IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug Housekeeping Housekeeping PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants