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

3.14.x: Prerelease publish failed to bump version in most package.json files. #2107

Closed
abernix opened this issue May 23, 2019 · 13 comments · Fixed by #2185
Closed

3.14.x: Prerelease publish failed to bump version in most package.json files. #2107

abernix opened this issue May 23, 2019 · 13 comments · Fixed by #2185

Comments

@abernix
Copy link

abernix commented May 23, 2019

This behavior is a scripted behavior which we've used for some time now, but seems to have broken with 3.14.1 (presumably 3.14.0 too, but we'd been using 3.13.4 before). Essentially, Lerna didn't bump the versions for the packages it said it was going to bump, committed the one that it did update (the very first one), and then went on to try and publish — ultimately failing because it attempts to publish over the previously published version.

Expected Behavior

When running the following, I expect it to suggest bumping all the appropriate packages by a prerelease faction, prompt with proposed versions, and when confirmed, publish them to their appropriate prerelease versions (e.g. .alpha.n, in this case):

lerna publish --exact \
	--force-publish=apollo-server-core \
	--include-merged-tags \
	--preid alpha \
	--dist-tag next \
	prerelease 

As I expected, this yielded a prompt of suggested version bumps, as it typically does:

Changes:
 - apollo-datasource-rest: 0.4.0 => 0.4.1-alpha.0
 - apollo-engine-reporting: 1.2.0-alpha.3 => 1.2.0-alpha.4
 - apollo-server-azure-functions: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-cloud-functions: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-cloudflare: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-core: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-express: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-fastify: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-hapi: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-integration-testsuite: 2.6.0-alpha.3 => 2.6.0-alpha.4 (private)
 - apollo-server-koa: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-lambda: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-micro: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server-plugin-base: 0.5.0-alpha.3 => 0.5.0-alpha.4
 - apollo-server-testing: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - apollo-server: 2.6.0-alpha.3 => 2.6.0-alpha.4
 - graphql-extensions: 0.7.0-alpha.3 => 0.7.0-alpha.4

Two of these packages have relevancy to this bug, but I've included the full output for accuracy. Of note are:

apollo-datasource-rest: 0.4.0 => 0.4.1-alpha.0

and

apollo-server-fastify: 2.6.0-alpha.3 => 2.6.0-alpha.4

Those will be relevant in the next section.

Current Behavior

The above typically works, publishing the proposed packages accordingly (barring registry failures), but it appears that some characteristics that I believe are new to 3.14.x prevented it from updating all of the packages and it instead only committed (to Git) the package.json bump to the version property for the very first package (in this case, apollo-datasource-rest), and then proceeded to attempt to publish the other packages without bumping the version in their package.json's, resulting in:

? Are you sure you want to publish these packages? Yes
lerna info execute Skipping GitHub releases
lerna info git Pushing tags...
lerna info publish Publishing packages to npm...
lerna info Verifying npm credentials
lerna http fetch GET 200 https://registry.npmjs.org/-/npm/v1/user 598ms
lerna http fetch GET 200 https://registry.npmjs.org/-/org/apollo-bot/package?format=cli 350ms
lerna http fetch PUT 403 https://registry.npmjs.org/apollo-server-fastify 569ms
lerna ERR! E403 You cannot publish over the previously published versions: 2.6.0-alpha.3

What's happened here is that it's tried publishing [email protected] (.3), even though it said it was going to bump to and publish [email protected] (.4).

Digging into the aftermath, I had the following commit on the tip of my branch (commit body expand-o'd in this <detail> for brevity):

The `Publish` commit

commit d25304e123f9176e1980e59e03ac57b03cf76f98
Author: Jesse Rosenberger <[email protected]>
Date:   Thu May 23 19:30:10 2019 +0300

    Publish
    
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]
     - [email protected]

diff --git a/packages/apollo-datasource-rest/package.json b/packages/apollo-datasource-rest/package.json
index 562d4c6b..6a7cfeb0 100644
--- a/packages/apollo-datasource-rest/package.json
+++ b/packages/apollo-datasource-rest/package.json
@@ -1,6 +1,6 @@
 {
   "name": "apollo-datasource-rest",
-  "version": "0.4.0",
+  "version": "0.4.1-alpha.0",
   "author": "[email protected]",
   "license": "MIT",
   "repository": {

Most of the version bumps are missing here, and it would have been expected to bump the other packages it thought it wanted to bump as well.

It had, however, gone ahead and git tagged all the packages though (snipped some tags for brevity, again):

(HEAD -> release-2.6.0, tag: [email protected], tag: [email protected], ... tag: [email protected], ...  tag: [email protected], origin/release-2.6.0)

And ultimately, it had published nothing. (I consider this a good thing, given its likely doomed fate!)

Steps to Reproduce (for bugs)

  1. lerna publish --exact \
      --force-publish=apollo-server-core \
      --include-merged-tags \
     --preid alpha \
     --dist-tag next \
     prerelease 
    
lerna.json

{
  "packages": ["packages/*"],
  "version": "independent"
}

Context

I realize that undoing a failure here is difficult and there might be a better way to undo the failure, but in this case, I wasn't sure as to the best approach. Typically, if it does the bumps and fails in an npm step, I'm still able to use from-git to use the committed details.

In this case, however, the commit was suggesting that a bunch of packages had been updated in its message but the versions were only bumped for one. I therefore deleted the new git tags both locally and on the remote server, my working tree to the previous (pre-Publish) commit via git reset --hard HEAD~ and tried the publish again.

Ultimately, I reverted to [email protected], tried again, and had success.

Your Environment

Executable Version
lerna --version 3.14.1
npm --version 6.9.0
node --version 10.15.1
OS Version
NAME VERSION
macOS Mojave 10.14.4

Happy to try and help however!

@evocateur
Copy link
Member

Do you have any lifecycle scripts in the root or leaves that git checkout ... anywhere? If they're run in version or postversion, I believe those will pollute (that is, break) the collection of proper changes.

In any case, I would very much appreciate a minimal reproduction to make progress on a diagnosis.

ulivz added a commit to vuejs/vuepress that referenced this issue Jun 4, 2019
abernix added a commit to apollographql/apollo-server that referenced this issue Jun 19, 2019
…pin.

> Note: This is an empty commit because I mis-referenced the Lerna PR in the
last commit (b42d92d).  I meant,
lerna/lerna#2107 but had written
lerna/lerna#2100.

In the same spirit as 840e2a7, this reverts
commit adde690 which Renovate re-upgraded
in #2810.

Therefore, this commit also puts a restriction within Renovate's
configuration to prevent further upgrades.  Unfortunately, this means we're
stuck at this version of Lerna until we get time to make a reproduction for
the issue I logged when this first cropped up in
lerna/lerna#2107, but I haven't yet found the
cycles to make that happen.

I guess our repository is a reliable reproduction, right now!

cc @trevor-scheer @JakeDawkins @evans
@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Jul 16, 2019

We have this same issue in Babel, but with a "normal" version (non a prerelease).

Version 7.5.0 had this problem; I had to manually update all the package.jsons
Version 7.5.1 to 7.5.4 worked fine.
Version 7.5.5 (which I want to release) has this same problem.

One thing that I noticed is that the package.json files which have been updated in the upcoming 7.5.5 and in 7.5.0 are the same.

I'm really sorry, but I'm going to give exactly the opposite of a minimal example. 😅 I'll also try to debug it and report anything I find.

Steps to reproduce:

  1. Fork https://github.com/babel/babel/
  2. git reset --hard d8da63c929f2d28c401571e2a43166678c555bc4 (and, if present, git tag --delete v7.5.5)
  3. git push -f, to also reset origin (and, if present, git push --delete origin v7.5.5)
  4. ./node_modules/.bin/lerna version --force-publish="@babel/runtime,@babel/runtime-corejs2,@babel/runtime-corejs3,@babel/standalone,@babel/preset-env-standalone"

We don't have any version or postversion script.


EDIT:

This is the --loglevel silly output: https://gist.github.com/nicolo-ribaudo/4f262771ae1fded3a6e4e5f95dd90f25
And these are the only updated files:

	modified:   lerna.json
	modified:   packages/babel-code-frame/package.json
	modified:   packages/babel-helper-fixtures/package.json
	modified:   packages/babel-helper-regex/package.json
	modified:   packages/babel-parser/package.json
	modified:   packages/babel-runtime-corejs3/package.json

Maybe it is a bug in how cycles are handled? Note that we only have cycles in dev dependencies.

@danez
Copy link

danez commented Jul 16, 2019

I think the issue lies in this function https://github.com/lerna/lerna/blob/master/utils/query-graph/query-graph.js#L62 which was introduced in 3.14.0

If I understand it correctly it gets all the leaf packages first (this._getNextLeaf()). If there are non left it tries to get the cycle packages BUT only if all that is left are cycle packages (this._onlyCyclesLeft()).

So the case we (babel) are running into is that after all the leaf packages are done, we have at least one package that is not a leaf (node.localDependencies.size>0), but it is also not part of any cycle (e.g. @babel/helper-replace-supers). In that case the function returns an empty array and I think at this point lerna stops updating any further packages and moves one to committing, thus leaving out a big part of our packages.
So I think that this case (packages which are not leafs but also not part of a cycle) is not handled correctly, but I just did a short debug session so I might have missed something though.

//cc @bweggersen

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Jul 16, 2019

I was going to write the same conclusion.

I'll try to open a PR.


EDIT

The bug is deeper: after markAsDone is called, @babel/helper-replace-supers should become a leaf node.


EDIT 2

I believe that this is what is happening: (the arrow means "depends on")
WhatsApp Image 2019-07-16 at 23 43 33

  1. F is a leaf node: it is processed and removed
  2. E has become a leaf node: it is processed and removed
  3. There aren't leaf nodes anymore (every node depends on something else), and there aren't only cycles: lerna stops.

EDIT 3

I was wrong, the previous graph was correctly handled. The problem is when a cycle ha a parent from two different nodes:

WhatsApp Image 2019-07-17 at 01 17 25

It only executes B

@bweggersen
Copy link
Contributor

Great debugging! I was able to repro the bug through a new test case. I'll look into it and create a PR with a fix. Thanks everyone!

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Jul 17, 2019

@bweggersen I was also working on a PR 😛
Btw, I think that it can still be made a little faster, because some "leaf cycles" can be executed in parallel with leaf nodes.
On the other hand, I belive that I'm making some things slower:
In case of A -> B -> C -> B it's safer to execute A _after B, C, like it would be dome if there wasn't a cycle.

@bweggersen
Copy link
Contributor

bweggersen commented Jul 17, 2019

Oh, I should have checked first! 🙃

I'm happy to let you drive the PR, if you want to, since you have pretty ASCII art :) Looking into the problem, this is what I came up with: master...bweggersen:bweggersen/cycle-with-common-parent

For transparency and clarity, this is my understanding of the order of execution (based on existing lerna functionality):
If there is an available leaf node, pick it. If not, and if we have cyclical nodes, then pick one of those (ref). Only start working on a cyclical node if we only have cyclical nodes left. As far as I understand, we have this constraint to make sure we don't start working on cyclical nodes while there are unbuilt dependencies of those cyclical nodes. It is possible that cyclical nodes are grandparents to nodes currently being worked on. So those nodes, and their parents, should be completed first. However, in situations like you illustrate in Edit 3, after having worked on B we have no available leaf node, but C and D are not alone either. We have an ordinary node in A. On my branch, I added a way to track tasks in progress. If we don't have any available leaf nodes, but we're not working on any tasks either, we can start picking up cyclical tasks, even if we have other ordinary nodes further up in the graph.

@evocateur Any feedback on what you think is the right thing to do here?

@nicolo-ribaudo
Copy link
Contributor

I am following an approach similar to this:

  1. When looking for cycles, collaps them to a single node. In the Edit 3 of 3.14.x: Prerelease publish failed to bump version in most package.json files. #2107 (comment), C-D would be collapsed to a single CD node without dependencies and whose only dependent is A.
  2. CD and B are both leaf nodes without dependencies; they can be executed.
  3. After both CD and B are done; A doesn't have any dependency left and can be executed.

I'm currently trying to make it work with nested cycles and cycles with intersection.

@evocateur
Copy link
Member

Honestly, all the graph traversal stuff is extremely out of my wheelhouse. I'm very appreciative of the expert help here.

@bweggersen I'm reviewing @nicolo-ribaudo's PR right now, I can take a look at your branch after my head stops spinning...

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Jul 17, 2019

Honestly, all the graph traversal stuff is extremely out of my wheelhouse. I'm very appreciative of the expert help here.

Don't worry, I had to re-write the logic three times because initially I had no idea about what I was doing 😅 Fortunately things became clearer while I was working on it.

Our approaches are completely different: I think that with my approach lerna respects the topological order better, but @bweggersen is way simpler.

@bweggersen If you could give an opinion about my PR it would be really appreciated!

@evocateur
Copy link
Member

@nicolo-ribaudo I really like your PR's improvements to the cyclic logging itself. Way more readable/actionable!

@evocateur
Copy link
Member

Fix is in v3.16.0, just released

@abernix
Copy link
Author

abernix commented Jul 24, 2019

Immense gratitude to those (especially @nicolo-ribaudo with the PR) who were able to jump in and get this resolved!

❤️

abernix added a commit to apollographql/apollo-server that referenced this issue Jul 24, 2019
Thanks to a gracious fix to Lerna by the Babel community, this issue
(originally reported in my lerna/lerna#2107) is no
longer an issue in new versions of Lerna, thanks to the fix provided in
lerna/lerna#2185.

Ref: lerna/lerna#2185
Ref: lerna/lerna#2107
abernix added a commit to apollographql/apollo-tooling that referenced this issue Sep 6, 2019
> In the same spirit as `apollo-server`:
> apollographql/apollo-server@585085d4d8dd7ab

Thanks to a gracious fix to Lerna by the Babel community, this issue
(originally reported in my lerna/lerna#2107) is no
longer an issue in new versions of Lerna, thanks to the fix provided in
lerna/lerna#2185.

Ref: lerna/lerna#2185
Ref: lerna/lerna#2107

cc @JakeDawkins
cc @trevor-scheer
trevor-scheer pushed a commit to apollographql/apollo-tooling that referenced this issue Sep 6, 2019
* Stop pinning Lerna to pre v3.14.0.

> In the same spirit as `apollo-server`:
> apollographql/apollo-server@585085d4d8dd7ab

Thanks to a gracious fix to Lerna by the Babel community, this issue
(originally reported in my lerna/lerna#2107) is no
longer an issue in new versions of Lerna, thanks to the fix provided in
lerna/lerna#2185.

Ref: lerna/lerna#2185
Ref: lerna/lerna#2107

cc @JakeDawkins
cc @trevor-scheer

* chore(deps): update dependency lerna to v3.16.4
devs-cloud pushed a commit to devs-cloud/vue-dev that referenced this issue Dec 27, 2019
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 a pull request may close this issue.

5 participants