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

Enable ydoc-server tests #12252

Merged
merged 7 commits into from
Feb 12, 2025
Merged

Enable ydoc-server tests #12252

merged 7 commits into from
Feb 12, 2025

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Feb 10, 2025

Pull Request Description

Enable ydoc-server tests after corepack is fixed in #12249

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@4e6 4e6 added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Feb 10, 2025
@4e6 4e6 self-assigned this Feb 10, 2025
@4e6 4e6 changed the title Enable ydoc-server Enable ydoc-server tests Feb 10, 2025
@4e6
Copy link
Contributor Author

4e6 commented Feb 10, 2025

IDK Windows build seems working 🤷

@4e6 4e6 marked this pull request as ready for review February 10, 2025 19:53
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

  • I am glad sbt doesn't require node & co. right now
    • when the dependency on node was introduced many of my build machines stopped building sbt buildEngineDistribution
    • the sbt build is more lightweight without node & co.
  • it is bad we don't execute ydoc-server integration tests at all, but
    • we don't use ydoc-server anywhere in the release right now
    • until work on Ydoc.js is resuscitated...
    • we just want to make sure, it continues to work
    • and it doesn't regress

As such, I suggest to

  • not enable ydoc-server for development at all
  • only execute ydoc-server/test on CI

I believe following patch:

enso/build_tools$ git diff
diff --git build_tools/build/src/engine/context.rs build_tools/build/src/engine/context.rs
index eb1296f548..bbb8278f62 100644
--- build_tools/build/src/engine/context.rs
+++ build_tools/build/src/engine/context.rs
@@ -305,6 +305,7 @@ impl RunContext {
         }
         if self.config.build_native_ydoc {
             tasks.push("ydoc-server/buildNativeImage");
+            tasks.push("ydoc-server/test");
         }
         if self.config.build_project_manager_package() {
             tasks.push("buildProjectManagerDistribution");

would be a great balance in achieving so:

  • the ydoc-server would remain tested with every commit
  • regular sbt usage wouldn't require node & co.
  • Dmitry (or anyone else working on Ydoc.js) could still use sbt ydoc-server/test locally

As such I am not approving this change, but feel free to proceed if you think it is better than my suggested solution.

@hubertp
Copy link
Collaborator

hubertp commented Feb 11, 2025

It's not only that. Not having ydoc-server in aggregate means that global commands, e.g. javafmtCheck, are not run regularly. This feels like a degradation in code standards.

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

If the CI is happy, I am OK with this change.

when the dependency on node was introduced many of my build machines stopped building sbt buildEngineDistribution.

That is because buildEngineDistribution depends on ydoc-server/compile. That can be removed, and at the same time, ydoc-server can be kept in the enso aggregate so that its tests and global commands like javafmtCheck still runs on that project but buildEngineDistribution does not run node.

not enable ydoc-server for development at all

I would keep that in the aggregate, but remove the dependency from buildEngineDistribution, if possible.

@Akirathan
Copy link
Member

Akirathan commented Feb 11, 2025

I am looking into the history of build.sbt and trying to determine whether buildEngineDistribution was even dependent on ydoc-server at all. It doesn't seem so. The last relevant change is when Jaroslav removed ydoc-server from the enso aggregate in https://github.com/enso-org/enso/pull/12201/files#diff-5634c415cd8c8504fdb973a3ed092300b43c4b8fc1e184f7249eb29a55511f91. So I am wondering: why buildEngineDistribution launched pnpm at all?? @4e6

GitHub
Pull Request Description

as #12192 (comment) states:
there is a need to extract values from EnsoMultiValue in the "builtin method prelude code"
to enable specializations based on type of...

@4e6
Copy link
Contributor Author

4e6 commented Feb 11, 2025

So I am wondering: why buildEngineDistribution launched pnpm at all?

@Akirathan buildEngineDistribution does not launch corepack pnpm. You can try it yourself by running

$ sbt 'clean; buildEngineDistribution' &| tee build-engine-distribution.log

@Akirathan
Copy link
Member

@4e6 It does not currently. But it used to. I remember that, and @JaroslavTulach as well. My question is: How is that even possible?

@4e6
Copy link
Contributor Author

4e6 commented Feb 11, 2025

We don't start the ydoc-server as a part of the language-server anymore. You can run it as a separate process though

@Akirathan
Copy link
Member

We don't start the ydoc-server as a part of the language-server anymore. You can run it as a separate process though

That is good to know. So even if ydoc-server is now part of enso aggregate, buildEngineDistribution will not invoke pnpm. So @JaroslavTulach does not have to be afraid of incorporating heavy-weight dependency in the build process.

@4e6
Copy link
Contributor Author

4e6 commented Feb 11, 2025

Although, I see another issue with running corepack pnpm i https://github.com/enso-org/enso/actions/runs/13249248686/job/37024762090?pr=12252#step:10:1788

INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: ERROR: C:/_bazel/external/rules_rust+/crate_universe/private/common_utils.bzl:55:13: Traceback (most recent call last):
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 	File "C:/_bazel/external/rules_rust+/crate_universe/extensions.bzl", line 990, column 37, in _crate_impl
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 		_generate_hub_and_spokes(
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 	File "C:/_bazel/external/rules_rust+/crate_universe/extensions.bzl", line 626, column 51, in _generate_hub_and_spokes
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 		splice_outputs = splice_workspace_manifest(
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 	File "C:/_bazel/external/rules_rust+/crate_universe/private/splicing_utils.bzl", line 170, column 19, in splice_workspace_manifest
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 		cargo_bazel_fn(
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 	File "C:/_bazel/external/rules_rust+/crate_universe/private/common_utils.bzl", line 88, column 23, in _execute
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 		return execute(
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 	File "C:/_bazel/external/rules_rust+/crate_universe/private/common_utils.bzl", line 55, column 13, in execute
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: 		fail(_EXECUTE_ERROR_MESSAGE.format(
 INFO ide_ci::program::command: sbt ℹ️ [info] . postinstall: Error in fail: Command [C:/_bazel/modextwd/rules_rust++crate/cargo-bazel.exe, "splice", "--output-dir", C:/_bazel/modextwd/rules_rust++crate/crates/splicing-output, "--splicing-manifest", C:/_bazel/modextwd/rules_rust++crate/crates/splicing_manifest.json, "--config", C:/_bazel/modextwd/rules_rust++crate/crates/config.json, "--cargo-lockfile", C:/runner/_work/enso/enso/Cargo.lock, "--cargo", "C:/_bazel/external/rules_rust++rust_host_tools+rust_host_tools/bin/cargo.exe", "--rustc", "C:/_bazel/external/rules_rust++rust_host_tools+rust_host_tools/bin/rustc.exe"] failed with exit code 256.

I'll try to see if it is SBT-related or not

GitHub
Enso Analytics is a self-service data prep and analysis platform designed for data teams. - Enable ydoc-server tests · d3e5f6b

@4e6
Copy link
Contributor Author

4e6 commented Feb 11, 2025

Although, I see another issue with running corepack pnpm i

it looks like it's just a Bazel build randomly failing

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Feb 12, 2025
@mergify mergify bot merged commit f815f49 into develop Feb 12, 2025
52 of 53 checks passed
@mergify mergify bot deleted the wip/db/ydoc-server-corepack branch February 12, 2025 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants