Skip to content

deno: remove esbuild use in tests, keep denort in a separate output#445232

Draft
06kellyjac wants to merge 4 commits intoNixOS:masterfrom
06kellyjac:maintenance/deno/drop_esbuild_tests
Draft

deno: remove esbuild use in tests, keep denort in a separate output#445232
06kellyjac wants to merge 4 commits intoNixOS:masterfrom
06kellyjac:maintenance/deno/drop_esbuild_tests

Conversation

@06kellyjac
Copy link
Member

  • deno: remove esbuild use in tests
  • deno: keep denort in a separate output
  • deno: leave doCheck defaults
  • deno: reduce usage of with lib;

Unblocks #423375
Sorry for the delay.

context:

#423375 (comment)

image

Keeping denort relates to the deno builder work in a separate PR


cc: @ofalvai

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

The version of esbuild needs to exactly match the version deno requires
for currently one integration test.
https://github.com/evanw/esbuild/blob/195e05c16f03a341390feef38b8ebf17d3075e14/cmd/esbuild/main.go#L206-L214

Updating esbuild in lockstep with deno releases isn't ideal and neither is
modifying the deno source and diverging from upstream.
A placeholder must be provided or the code which mocks an npm registry
will fail for over 260 tests which use it.
Previously `denort` was built but removed from the final output.
Now it's available in a separate `rt` output to avoid bloating the main `out`
but also prevent rebuilding `deno` just for `denort`.
Deno is restricted by meta.platforms so shouldn't need doCheck
to also be restricted to match.
@06kellyjac 06kellyjac requested a review from ofalvai September 22, 2025 14:20
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. labels Sep 22, 2025
Copy link
Contributor

@ofalvai ofalvai left a comment

Choose a reason for hiding this comment

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

This looks like a fine compromise, thanks for doing the research. Also thank you for the misc cleanups!

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Sep 27, 2025
@ofalvai
Copy link
Contributor

ofalvai commented Sep 27, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 445232
Commit: d2ea5465fc0af11aa8bbc358d03333716dea32f4


aarch64-darwin

❌ 9 packages failed to build:
  • deno
  • deno.rt
  • era
  • gleam
  • quarto
  • quartoMinimal
  • rstudio
  • rstudioWrapper
  • silverbullet

Error logs: `aarch64-darwin`
deno
[bundle_test 000.78] 
[bundle_test 000.78] 
[bundle_test 000.78] thread '<unnamed>' panicked at cli/tools/bundle/mod.rs:560:8:
[bundle_test 000.78] called `Result::unwrap()` on an `Err` value: channel closed
[bundle_test 000.78] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[bundle_test 000.78] bundle: basic in-memory bundle succeeds and returns content ...

thread 'js_unit_tests::bundle_test' panicked at tests/integration/js_unit_tests.rs:246:3:
assertion left == right failed: Deno should have exited cleanly
left: Some(0)
right: Some(1)
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

failures:
js_unit_tests::bundle_test

test result: FAILED. 795 passed; 1 failed; 12 ignored; 0 measured; 60 filtered out; finished in 90.35s

error: test failed, to rerun pass -p cli_tests --test integration_tests

@ofalvai
Copy link
Contributor

ofalvai commented Sep 27, 2025

@06kellyjac is this test supposed to work with the fake esbuild we provide?

deno> [bundle_test 000.78]
deno> [bundle_test 000.78]
deno> [bundle_test 000.78] thread '<unnamed>' panicked at cli/tools/bundle/mod.rs:560:8:
deno> [bundle_test 000.78] called `Result::unwrap()` on an `Err` value: channel closed
deno> [bundle_test 000.78] note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
deno> [bundle_test 000.78] bundle: basic in-memory bundle succeeds and returns content ...
deno>
deno> thread 'js_unit_tests::bundle_test' panicked at tests/integration/js_unit_tests.rs:246:3:
deno> assertion `left == right` failed: Deno should have exited cleanly
deno>   left: Some(0)
deno>  right: Some(1)
deno> note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
deno>
deno>
deno> failures:
deno>     js_unit_tests::bundle_test

@06kellyjac
Copy link
Member Author

Since it's just a placeholder file we probably need to skip this too.
I'm suprised this only fails on darwin, or maybe I need to re-run things.


An alternative would be to have a pinned copy of esbuild but we should also patch deno so it uses our provided copy rather than downloading its own copy.
Maybe if I check the source it looks for a copy on the path with the exact matching version first? But I doubt it.

@06kellyjac
Copy link
Member Author

Mmm, yeah I was looking at dropping this because bundling was pretty officially on the way out, but now it's back in 2.4 we probably need to provide a matching esbuild.

https://deno.com/blog/v2.4

@06kellyjac 06kellyjac marked this pull request as draft September 29, 2025 12:06
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 30, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". label Jan 8, 2026
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge-bot eligible This PR can be merged by commenting "@NixOS/nixpkgs-merge-bot merge". label Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants