Refactor: migrate to batching toposort for execution of releases - #46
Refactor: migrate to batching toposort for execution of releases#46ThisIsMissEm wants to merge 2 commits into
Conversation
6c41356 to
b3ca7bb
Compare
ThisIsMissEm
left a comment
ThisIsMissEm
left a comment
There was a problem hiding this comment.
Left some comments about particular aspects that might be curious for reviewers.
|
|
||
| // Return the notes. | ||
| return notes.join("\n\n"); | ||
| return notes.join("\n").replace("\n\n", "\n").trim(); |
There was a problem hiding this comment.
This may not be needed, but I was noticing that our release notes contained a lot of unnecessary new lines
| if (p._nextType) return true; | ||
| // 2. Any local dep package has local deps that have changed. | ||
| else if (hasChangedDeep(p._localDeps, [...ignore, ...packages])) return true; | ||
| else if (p._localDeps && hasChangedDeep(p._localDeps, [...ignore, ...packages])) return true; |
There was a problem hiding this comment.
I came across this bug originally when doing a spike into why our API usage of multi-semantic-release didn't work, essentially, p._localDeps can be undefined, as far as I can tell, so this check prevents recursion which would result in an error.
There was a problem hiding this comment.
Good catch. I've moved localDeps resolver one level up. 48bcad7
| // Call semanticRelease() on the directory and save result to pkg. | ||
| // Don't need to log out errors as semantic-release already does that. | ||
| pkg.result = await semanticRelease(options, { | ||
| pkg.result = semanticRelease(options, { |
There was a problem hiding this comment.
by not awaiting here, we allow parallel releases of packages within a given batch; this does mean an interleaved log at the moment, but does mean that you should have faster releases.
| releases: result.releases, | ||
| }); | ||
|
|
||
| pkg.isPending = false; |
There was a problem hiding this comment.
isPending was added when I was modifying the original algorithm of the synchronizer, so can safely be ignored now — I was using this to determine the todo()'s instead of the presence or absence of the pkg.result property
|
Hey, @ThisIsMissEm, Great improvement. This is really cool, keep going. https://github.com/semantic-release/git/blob/905f113a577c55cd9bb0a37ea3504d9e8ee2dfa2/lib/git.js#L11 async function getModifiedFiles(execaOptions) {
return (await execa('git', ['ls-files', '-m', '-o'], execaOptions)).stdout
.split('\n')
.map(file => file.trim())
.filter(file => Boolean(file));
} |
|
Isn't this just executing in a subtree of the repository? |
|
e.g., $ pwd
[...]/repo/packages/example-create-react-app
$ git rev-parse --show-cdup
../../
$ git st
## develop...origin/develop
M package.json
M ../react/package.json
M ../../scripts/createOrUpdateReleasePR.js
$ git ls-files -m -o
build/ [lots of files]
node_modules/ [lots of files]
package.json
$ git ls-files -m
package.json |
Honestly, not sure there. How shall we move forwards on this? |
|
It just seemed like a similar problem was being solved. Anyway, I suppose everything can be made easier. We need just number the vertices of the deps graph, right? And follow the marks during publication. So we could build an incidence matrix/adjacency matrix, find the first kind sources/entry points (they have no outgoing arcs), number them. After that, we may exclude these vertices, remove incoming arcs, and repeat the procedure. With cyclic graphs, I think, we can use any package that needs release as the entry point. |
|
Not sure if this pull-request is still alive...
Yes it could. The test definition is actually fine. Processing wise: the trick is to keep a trace of packages that you analyzed when traversing the graph. Skip the ones you already processed earlier to prevent endless loops. If a depends on b, and b depends on a, they should simply be updated both if any of them has a change. |
Right, which would make your release process either go indefinitely or you'd always have one package not published as fully up to date. |
|
At some point I'll get back to this (I was actually hacking on semantic-release and MSR the other day, but unfortunately the company that was paying for that effort is no longer) |
|
I know this is rather old, but we are very interested in having this. @ThisIsMissEm Could I help in any way getting this over the line? Since it's been a while now, has there been any other alternatives to this adopted by the wider community? The only thing I could dig out that comes close to having ordered releases is https://github.com/semrel-extra/zx-bulk-release which uses |
|
I'm unlikely to pick this up again right now, unless there's sponsorship, as I simply don't have the time otherwise. |
|
Just another one example, if somebody is willing to continue this work: https://github.com/qiwi/multi-semantic-release/blob/master/lib/multiSemanticRelease.js |
|
@antongolub Thanks for sharing that. Seems like the |
|
Exactly. But @qiwi fork has many tiny subtle differences, I just can't rebase it and avoid regressive bugs. |
4 participants
This is still heavily a WIP, but from the API side, it seems to work well. The tests are currently failing because there's a cyclic dependency chain in the
tests/fixtures/yarnWorkspacespackages, which, as far as I can tell could never actually be released fully, as there'd always be something out of date.I guess it depends on what actually is considered for the DAG, right now we're considering all dependencies, but maybe only
dependenciesshould count, asdevDependencies,optionalDependenciesandpeerDependenciesdon't really affect versioning as far as I can tell?Consider this a proof of concept, and we can continue polishing it — e.g., I'm not sure updating the package objects is ideal, and instead perhaps we're better just returning a results object? I'm not sure.