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

Always set exact version when publishing #8880

Merged

Conversation

Niklas-Dahlquist
Copy link
Contributor

@Niklas-Dahlquist Niklas-Dahlquist commented Dec 17, 2020

What it does

The patch promotes usage of a specific version number for each future Theia publication. Solves issue #8879.

We add use of the --exact option to lerna, see this link:
https://github.com/lerna/lerna/tree/main/commands/version#--exact

Fixes #8879

How to test

Run $ yarn publish:latest
Observe that all Theia extensions uses a specific version number on Theia dependencies.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the proposal feature proposals (potential future features) label Dec 17, 2020
@@ -68,7 +68,7 @@
"rebuild:electron": "theia rebuild:electron",
"rebuild:electron:debug": "DEBUG=electron-rebuild && yarn rebuild:electron",
"publish": "yarn && yarn test && yarn publish:latest",
"publish:latest": "lerna publish && yarn publish:check",
"publish:latest": "lerna publish --exact && yarn publish:check",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Niklas-Dahlquist I could not find any information for the --exact flag in the lerna publish documentation:

Can you perhaps include a link to documentation for the --exact flag in the commit message, and/or pull-request description?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Vince!

The information for --exact is hidden under lerna version, see this direct link

/Niklas

Copy link

@Oleksandr1201vsp Oleksandr1201vsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niklas-Dahlquist:nidah/exact_version

Copy link

@Oleksandr1201vsp Oleksandr1201vsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛳️

@paul-marechal
Copy link
Member

paul-marechal commented Jan 6, 2021

@eclipse-theia/core is anyone forseing issues with setting dependencies to Theia extensions as x.y.z rather than ^x.y.z?

I am in favor of this change.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jan 7, 2021 via email

@paul-marechal
Copy link
Member

paul-marechal commented Jan 7, 2021

@marcdumais-work when using the main @theia/* packages you shouldn't have to do anything as long as you use all the same versions in your application's package.json. Only if you start using mismatching Theia extensions that are hard coded to depend on different versions will you need to use resolutions to force using a single version.

@Niklas-Dahlquist
Copy link
Contributor Author

The problem is if we are depending on an older release depending on Theia extensions we break unless we have specified a resolution with fixed versions of all Theia extensions in the entire dependency tree. (both direct and indirect dependencies).
See comment in #8879 (comment)

@marcdumais-work
Copy link
Contributor

@marcdumais-work when using the main @theia/* packages you shouldn't have to do anything as long as you use all the same versions in your application's package.json. Only if you start using mismatching Theia extensions that are hard coded to depend on different versions will you need to use resolutions to force using a single version.

I understand, but my concern is that, in practice, all non-trivial Theia app will end-up having some mismatched Theia extensions (framework and/or 3rd party), and so it will become mandatory to bolt-down everything in a resolutions block.

The way we currently do things, though not ideal from all angles, does afford us (and other Theia extensions developers) the leeway to lazily release theia extensions. E.g. we take advantage of that for @theia/cpp-debug, that has not been released since Theia 1.0.0 but continues to work because it depends on latest theia packages that are still API-compatible.


Would it be fair to describe the situation like so:

"with this new proposed dependencies handling, any and all Theia extensions part of an application will need to depend on the exact same @Theia framework extensions version".

or

"alternatively use a resolutions block to force all @Theia extensions (explicitly pulled or not)) to be of a same specific version "

@Niklas-Dahlquist
Copy link
Contributor Author

Take this example, a simple application that depends on an old Theia release will no longer work without a resolutions block:

"dependencies": {
	"@theia/core": "1.3.0",
	"@theia/editor": "1.3.0"
}

Create that then run yarn followed by "yarn why @theia/core" and you will see that you have multiple instances.
If the PR is merged the above example will no longer require a resolutions block.

In the other case where we have conflicting dependency versions from our in-house extensions, then we need to either fix these conflicts or use resolutions.
With the PR the resolutions only need to specify direct dependencies.
Without the PR the resolutions need to specify direct and in-direct dependencies.

@marcdumais-work
Copy link
Contributor

Niklas, do you see a way we could validate this? I'd like to be able to see how this would work, one and two releases down the road, since some of the side-effects will not be immediately visible. This would also permit knowing for sure what to expect, and so being able to guide adopters / extension developers in the transition.

If you do not have a better idea, I would suggest simulating this using a local npm registry, doing a couple of simulated releases and then using the local registry to build various Theia apps and extensions.

@marcdumais-work
Copy link
Contributor

(sorry - momentarily closed PR by accident)

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there will be quite important impacts for Theia app developers and probably Theia extensions developers too. This will require an entry in CHANGELOG documenting the breakage.

@Niklas-Dahlquist
Copy link
Contributor Author

We tested this by using an old "next" release since they are already published with --exact.

See this example:

Correct behaviour with "next" releases which uses exact version numbers

-- older 'next' release (with exact version numbers)
"dependencies": {
"@theia/core": "1.10.0-next.2eb001e3",
"@theia/editor": "1.10.0-next.2eb001e3"
}

$ yarn why @theia/core | grep Found
=> Found "@theia/core@1.10.0-next.2eb001e3"

Incorrect behaviour with an old official/stable release

-- older official release (without exact version numbers)
"dependencies": {
"@theia/core": "1.8.0",
"@theia/editor": "1.8.0"
}

$ yarn why @theia/core | grep Found
=> Found "@theia/core@1.8.0"
=> Found "@theia/editor#@theia/core@1.9.0"
=> Found "@theia/variable-resolver#@theia/core@1.9.0"

"next" releases already uses --exact and it works, we don't foresee any issues with enabling the --exact option also for official/stable releases.

@marcdumais-work
Copy link
Contributor

Thanks for the example @Niklas-Dahlquist. I'll play a bit with next versions and comment-back.

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Jan 13, 2021

My observations:

I started with the theia-electron example app.

  1. confirm it builds/runs
    git clean -ffdx && yarn cache clean && yarn && yarn build && yarn start
    => yes, it builds and starts

  2. Switch @Theia extensions from "latest" to a recent but not most recent "next": "1.10.0-next.2eb001e3" and add "@theia/cpp-debug": "next" extension as a dependency, that has a release cycle not tied to the main repo . It depends on the "next" @theia/core and @theia/debug.

"dependencies": {
        "@theia/callhierarchy": "1.10.0-next.2eb001e3",
        "@theia/cpp-debug": "next",
        "@theia/core": "1.10.0-next.2eb001e3",
        "@theia/editor": "1.10.0-next.2eb001e3",
[...]

git clean -ffdx && yarn cache clean && yarn && yarn build && yarn start
=> the app still builds but does not start (probably) because two versions of some Theia extensions are present and the Inversify error this triggers. The console is not accessible so I can't 100% confirm why, but we can confirm another way:

as expected 2 versions of @theia/core and possibly more are pulled:

$ yarn why @theia/core
yarn why v1.22.10
[1/4] Why do we have the module "@theia/core"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "@theia/core@1.10.0-next.2eb001e3"
info Has been hoisted to "@theia/core"
info Reasons this module exists
   - Specified in "dependencies"
   - Hoisted from "@theia#variable-resolver#@theia#core"
[...]
info Disk size without dependencies: "11.09MB"
info Disk size with unique dependencies: "33.67MB"
info Disk size with transitive dependencies: "53.55MB"
info Number of shared dependencies: 153
=> Found "@theia/cpp-debug#@theia/core@1.10.0-next.db4b2e71"
info Reasons this module exists
   - "@theia#cpp-debug" depends on it
   - Hoisted from "@theia#cpp-debug#@theia#debug#@theia#core"
 [...]
info Disk size without dependencies: "10.67MB"
info Disk size with unique dependencies: "33.25MB"
info Disk size with transitive dependencies: "53.13MB"
info Number of shared dependencies: 153
Done in 1.21s.
  1. add resolutions so that @theia/cpp-debug's Theia dependencies pull the same version as the rest of the app:
"resolutions": {
        "**/fs-extra": "^4.0.3",
        "@theia/core": "1.10.0-next.2eb001e3",
        "@theia/debug": "1.10.0-next.2eb001e3"
    },

git clean -ffdx && yarn cache clean && yarn && yarn build && yarn start
=> works again


@Niklas-Dahlquist any surprises in the above? Is there a better way or did I work-around the issue "correctly", the same we'll suggest adopters do, when this is merged? (edit: transposed to latest, or course - it already works like this for next)

@Niklas-Dahlquist
Copy link
Contributor Author

@marcdumais-work No surprises in the crash nor the way you resolved the issues using resolutions.

When conflicts occur the options that exists are to remove/fix the conflicting dependencies in the extension causing the duplication or hide the duplication of extensions by using a resolutions statement forcing the use of same versions like in the example above.

@marcdumais-work
Copy link
Contributor

Thanks for confirming @Niklas-Dahlquist.

Would you be available to come briefly present and answer questions about this PR at our next dev-meeting? I'd like to get community buy-in.

@marcdumais-work
Copy link
Contributor

@thegecko
Copy link
Member

@arekzaluski

@marcdumais-work
Copy link
Contributor

For my own peace of mind I have simulated a Theia release based on this PR (usinga local registry) and built the example app from the theia-docker example (just the app, not the Docker image). I have done this using Gtipod

Here are the instructions

open this PR is Gitpod: https://gitpod.io/#https://github.com/eclipse-theia/theia/pull/8880

Then kill the example app started automatically and in a terminal do:

$> npm add -g verdaccio
$> verdaccio
#in another terminal: 
$> npm adduser --registry http://localhost:4873
#(is there a way to supply user/pad/email programatically?: user/user/user@user.com)
# publish new Theia release to local registry, based on this PR:
$> yarn && yarn test:theia && npx lerna publish --registry http://localhost:4873  
# test building a Theia app from what we published:
$> cd /workspace && git clone https://github.com/theia-ide/theia-apps.git && cd theia-apps/theia-docker && cp latest.package.json package.json && yarn --registry http://localhost:4873 && yarn theia build && yarn theia start --cwd example-electronp --hostname=0.0.0.0

The resulting application appears to work fine

image

@marcdumais-work
Copy link
Contributor

@Niklas-Dahlquist based on the above and the fact that I did not need to make changes to the example application, I think we can remove the CHANGELOG entry, unless you think otherwise.

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@marcdumais-work
Copy link
Contributor

I have added a dev-meeting entry for this, for the dev-community's information.

Theia is using "^1.x.y" in all modules, this causes two problems
1) Since romantic versioning is used within Theia repo, API
   breakage might be introduced between any version

2) It's harder to build applications using old versions of Theia
   extensions since if not every needed Theia extension is explicitly
   stated in dependencies you will likely end up with pulling several
   parallel versions of @theia/core for example. Due to dependency
   injection we can not have multiple parallel versions of the same
   Theia extension module

Fixes issue eclipse-theia#8879 when publishing next version

Contributed by STMicroelectronics

Signed-off-by: Niklas DAHLQUIST <niklas.dahlquist@st.com>
Signed-off-by: Samuel HULTGREN <samuel.hultgren@st.com>
@Niklas-Dahlquist
Copy link
Contributor Author

Niklas-Dahlquist commented Jan 19, 2021

I did an additional test:

Published a 1.10.0 as described above

yarn && yarn test:theia && npx lerna publish --registry http://localhost:4873

Created a 1.10.1 with the --exact option

yarn && yarn test:theia && npx lerna publish --exact --registry http://localhost:4873

Created a 1.11.0 with the --exact option

yarn && yarn test:theia && npx lerna publish --exact --registry http://localhost:4873

Cloned the same application

cd /workspace && git clone https://github.com/theia-ide/theia-apps.git && cd theia-apps/theia-docker 

Created 1.10.0.package.json where I replaced all versions with 1.10.0

cp 1.10.0.package.json package.json && yarn --registry http://localhost:4873 && yarn theia build && yarn theia start --cwd example-electronp --hostname=0.0.0.0

The Theia application never leaves the spinning wheel of ....

$ yarn why @theia/debug | grep Found
=> Found "@theia/debug@1.10.0"
=> Found "@theia/plugin-ext#@theia/debug@1.11.0"
=> Found "@theia/plugin-ext-vscode#@theia/debug@1.11.0"
=> Found "@theia/vsx-registry#@theia/debug@1.11.0"

Created 1.10.1.package.json where I replaced all versions with 1.10.1

cp 1.10.1.package.json package.json && yarn --registry http://localhost:4873 && yarn theia build && yarn theia start --cwd example-electronp --hostname=0.0.0.0

The Theia application starts normally!!!

$ yarn why @theia/debug | grep Found
=> Found "@theia/debug@1.10.1"

Created caret1.10.1.package.json where I replaced all versions with ^1.10.1

cp caret1.10.1.package.json package.json && yarn --registry http://localhost:4873 && yarn theia build && yarn theia start --cwd example-electronp --hostname=0.0.0.0

The Theia application starts normally!!!

yarn why @theia/debug | grep Found
=> Found "@theia/debug@1.11.0"

Conclusion use --exact when publishing....

I agree that the change isn't breaking, still kept the CHANGELOG item since we need to know when we abandoned caret versioning.

@marcdumais-work
Copy link
Contributor

Nice test - thanks Niklas :)

@Niklas-Dahlquist
Copy link
Contributor Author

Hi Sorry we mixed up the day for the dev meeting days - Regarding the minutes and the question "Please post on the PR if you'd like more time to test - else we'll aim to merge it tomorrow"

We don't aim to do more tests, the fix solves an issue and is ready to merge.

@marcdumais-work
Copy link
Contributor

No one has posted objections, so let's merge.

@marcdumais-work marcdumais-work merged commit 1e3328e into eclipse-theia:master Jan 20, 2021
@Niklas-Dahlquist Niklas-Dahlquist deleted the nidah/exact_version branch January 21, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal feature proposals (potential future features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Theia application builds should be easy to reproduce
6 participants