chore: simplify version script invocation

[pichlermarc]

Which problem is this PR solving?

Currently we need the cross-var dependency to support Windows, and would also need lerna in every package as we're using it in the version script. It seems to me that the version script can just be invoked directly from the precompile script.

Related discussion #3885
Related PR adding cross-var #3857

Short description of the changes

• invokes version from precompile in every package
• removes cross-var as it is then not needed anymore

Type of change

• internal

How Has This Been Tested?

• Locally running npm run compile from project root -> generated version.ts correctly for all modules
• Locally running npm run compile from module root -> generated version.ts correctly for module

[codecov]

Codecov Report

Merging #3887 (c77d145) into main (95a5e0c) will not change coverage.
The diff coverage is n/a.

❗ Current head c77d145 differs from pull request most recent head f29dc4e. Consider uploading reports for the commit f29dc4e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3887   +/-   ##
=======================================
  Coverage   92.88%   92.88%           
=======================================
  Files         297      297           
  Lines        8836     8836           
  Branches     1814     1814           
=======================================
  Hits         8207     8207           
  Misses        629      629           

[dyladan]

The reason we didn't do this before is because if i'm in some subpackage and run npm run version it will only run for the current package and not my package's dependencies. The most common issue was building a package in a fresh checkout and it would fail because version.ts was missing in some dependency.

[dyladan]

We could remove cross-var by doing this:

Suggested change

	"precompile": "npm run version",
	"precompile": "lerna run version --scope @opentelemetry/api --include-dependencies",

Since lerna is already in the root and is a hoisted dependency I don't think it should be a problem to keep it as long as all subpackages contain the same version.

[pichlermarc]

Initially I was not trying to remove cross-var, but what initially sparked the idea was a devDependency on lerna in @opentelemetry/core but not anywhere else which I wanted to get rid of, but then I saw what I thought was an opportunity to remove cross-var as well 😅

I guess this leaves us with two options - see comment

[pichlermarc]

The reason we didn't do this before is because if i'm in some subpackage and run npm run version it will only run for the current package and not my package's dependencies. The most common issue was building a package in a fresh checkout and it would fail because version.ts was missing in some dependency.

Ah, that's the gotcha I was waiting for 😓

I guess that leaves us with two options:

• add lerna as a devDependency in every package (as was the initially suggested in feat: new merge function #2484 (comment))
• invoke version directly and make lerna run version be part of postinstall in the top-level package.json as update-ts-configs is today.

I'd lean towards option #1 WDYT @dyladan?

[dyladan]

The reason we didn't do this before is because if i'm in some subpackage and run npm run version it will only run for the current package and not my package's dependencies. The most common issue was building a package in a fresh checkout and it would fail because version.ts was missing in some dependency.

Ah, that's the gotcha I was waiting for 😓

I guess that leaves us with two options:

• add lerna as a devDependency in every package (as was the initially suggested in feat: new merge function #2484 (comment))
• invoke version directly and make lerna run version be part of postinstall in the top-level package.json as update-ts-configs is today.

I'd lean towards option #1 WDYT @dyladan?

I also prefer option 1

[pichlermarc]

I also prefer option 1

Alright then I'll close this one and open another PR that does that instead. 🙂