Replace script_plugins with a clippy like rustc driver (named crown) by sagudev · Pull Request #30508 · servo/servo

sagudev · 2023-10-06T08:41:36Z

Required by rust-lang/rust#116412, modeled after https://github.com/Rust-for-Linux/klint.

If crown is not used in compilation of script crate we get this warning:

warning: unknown lint: `crown_is_not_used`
  --> components/script/lib.rs:16:8
   |
16 | #[deny(crown_is_not_used)]
   |        ^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> components/script/lib.rs:15:8
   |
15 | #[warn(unknown_lints)]
   |        ^^^^^^^^^^^^^

Sccache does not work when using crown, but that is sccache issue that I am actively working on.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
There are tests for these changes

sagudev · 2023-10-06T09:27:56Z

If crown is not used we get this warning:

warning: unknown lint: `crown_is_not_used`
  --> components/script/lib.rs:16:8
   |
16 | #[deny(crown_is_not_used)]
   |        ^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> components/script/lib.rs:15:8
   |
15 | #[warn(unknown_lints)]
   |        ^^^^^^^^^^^^^

python/servo/platform/base.py

sagudev · 2023-10-07T10:47:29Z

@bors-servo try

github-actions · 2023-10-07T10:47:40Z

🔨 Triggering try run (#6440698317) with platform=all and layout=all

mrobinson · 2023-10-09T10:19:30Z

.github/workflows/linux.yml

  SHELL: /bin/bash
  SCCACHE_GHA_ENABLED: "true"
-  RUSTC_WRAPPER: "sccache"
+  #RUSTC_WRAPPER: "sccache"


Perhaps this was left by mistake?

No, sccache issue is still not resolved, but I have a fix PR maybe we can use it.

I would remove this line then, rather than leaving it commented out.

Let's remove this for now.

sagudev · 2023-10-10T17:45:35Z

I am getting this on mac:

gtar: target/release/*.dylib: Cannot stat: No such file or directory

Maybe there is some silent fail in mach and things still continue to roll after it.

sagudev · 2023-10-31T09:39:12Z

I fixed mac build, all real dylibs were under target/release/libs/*dylibs
only dylib under target/release is libscript_plugins.dylib that was used by rustc during compilation of script crate.

Sccache is still disabled.

components/script/lib.rs

python/servo/command_base.py

sagudev · 2023-11-02T05:53:11Z

Rebased for new nightly

mrobinson

This is a really nice effort. There is a bit more work to do here, but this is a great start.

mrobinson · 2023-11-17T08:00:13Z

.github/workflows/linux.yml

  SHELL: /bin/bash
  SCCACHE_GHA_ENABLED: "true"
-  RUSTC_WRAPPER: "sccache"
+  #RUSTC_WRAPPER: "sccache"


I would remove this line then, rather than leaving it commented out.

.github/workflows/mac.yml

python/servo/command_base.py

mrobinson · 2023-11-17T08:06:06Z

python/servo/platform/base.py

+    def install_crown(self, force: bool) -> bool:
+        if subprocess.call(["cargo", "install", "--path", "support/crown"],
+                           stdout=subprocess.PIPE, stderr=subprocess.PIPE) != 0:
+            raise EnvironmentError("Installation of crown failed.")
+
+        return True
+


There's an issue here which is that updates to crown won't be automatically installed when building.

That's why it's always forced. It would be nice if we could be more smart about it, but I do not have any idea how.

Perhaps when crown starts it could check a value in the workspace Cargo.toml which specifies the minimum version. If it is less than the current version it could emit an error and request running ./mach bootstrap again. ./mach bootstrap could always try to update to the latest version of `crown.

This could work, but crown is run for every rustc invocation, which could slow down compilation.

Maybe storing good rust version in env var would speed things up, but then we can have old data in env var.

support/crown/tests/compile_test.rs

support/crown/tests/ui/trace_in_no_trace_composable_works.rs

support/crown/src/common.rs

sagudev · 2023-11-17T09:47:51Z

Rebase

mrobinson · 2023-11-27T13:04:31Z

.github/workflows/mac.yml

          NIGHTLY_REPO: ${{ github.repository_owner }}/servo-nightly-builds
      - name: Build package for target
-        run: gtar -czf target.tar.gz target/${{ inputs.profile }}/servo target/${{ inputs.profile }}/*.dylib target/${{ inputs.profile }}/lib/*.dylib resources
+        run: gtar -czf target.tar.gz target/${{ inputs.profile }}/servo target/${{ inputs.profile }}/lib/*.dylib resources


Please remove this change line since it's no longer needed after the rebase.

I do not think so:

servo/.github/workflows/mac.yml

Line 139 in d10688b

run: gtar -czf target.tar.gz target/${{ inputs.profile }}/servo target/${{ inputs.profile }}/*.dylib target/${{ inputs.profile }}/lib/*.dylib resources

target/${{ inputs.profile }}/*.dylib is only matched with script_plugin.dylib and now that it does not exist this step fails.

mrobinson · 2023-11-27T13:15:51Z

.github/workflows/windows.yml

+        env:
+          RUSTC: "rustc"


Why is this necessary?

when doing cargo install --path support/crown we need to override RUSTC back to rustc otherwise cargo would use RUSTC=crown from .cargo/config.toml (we install crown without already having crown).

I think this should probably have a comment explaining this. It's a bit confusing otherwise. Thanks!

components/script/lib.rs

github-actions · 2023-12-01T10:54:32Z

🔨 Triggering try run (#7059079398) with platforms=windows and layout=none

github-actions · 2023-12-01T11:02:09Z

⚠️ Try run (#7059079398) failed.

github-actions · 2023-12-01T11:05:08Z

🔨 Triggering try run (#7059184213) with platforms=windows and layout=none

github-actions · 2023-12-01T11:11:46Z

⚠️ Try run (#7059184213) failed.

sagudev · 2023-12-01T11:13:35Z

⚠️ Try run (#7059184213) failed.

I always forgot that CI changes are not testable with this, but it works on windows for me: https://github.com/sagudev/cargo-crown/actions/runs/7059052531

servo-tidy.toml

…ervo#30508) * Remove script_plugins * Use crown instead of script_plugins * crown_is_not_used * Use crown in command base * bootstrap crown * tidy happy * disable sccache * Bring crown in tree * Install crown from tree * fix windows ci * fix warning * fix mac libscript_plugins.dylib is not available anymore * Update components/script/lib.rs Co-authored-by: Martin Robinson <mrobinson@igalia.com> * Update for nightly-2023-03-18 Mostly just based off servo#30630 * Always install crown it's slow only when there is new version * Run crown test with `mach test-unit` * Small fixups; better trace_in_no_trace tests * Better doc * crown in config.toml * Fix tidy for real * no sccache on rustc_wrapper * document rustc overrides * fixup of compiletest * Make a few minor comment adjustments * Fix a typo in python/servo/platform/base.py Co-authored-by: Samson <16504129+sagudev@users.noreply.github.com> * Proper test types * Ignore tidy on crown/tests --------- Co-authored-by: Martin Robinson <mrobinson@igalia.com>

Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.

This was previously disabled in servo#30508 due to sccache not working well with crown. The sccache issue (mozilla/sccache#861) linked in that PR is now closed and [testing][1] on my fork also seems to indicated we should be able to turn on sccache again. [1]: https://github.com/mukilan/servo/actions/runs/9154196647 Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>

This was previously disabled in #30508 due to sccache not working well with crown. The sccache issue (mozilla/sccache#861) linked in that PR is now closed and [testing][1] on my fork also seems to indicated we should be able to turn on sccache again. [1]: https://github.com/mukilan/servo/actions/runs/9154196647 Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>

Copy from <rust-lang#10469 (comment)>: > I've never been entirely clear why it does this. rust-lang#4006 didn't really > explain why it added the corresponding host_dylib_path. I can't envision > a scenario where it matters. I think compiler plugins and proc-macros > should load just fine, since libstd.so should already be loaded by the > compiler. Also, rustc uses rpath these days, and on Windows libstd.so is > placed in the bin directory which will be searched first anyways. > > On balance, I think it should be safe to just remove sysroot_host_libdir. > I can't come up with a scenario where it matters, at least on > windows/macos/linux. One issue is that this is most likely to affect > plugins, but those are deprecated and I think only Servo was the real > holdout. A concern is that nobody is going to test this use case before > it hits stable. Also, * compiler plugins were removed rust-lang/rust#116412 * servo has moved off from plugins: servo/servo#30508 So should generally be fine.

sagudev mentioned this pull request Oct 6, 2023

Remove support for compiler plugins. rust-lang/rust#116412

Merged

sagudev force-pushed the crown branch from dfb8c03 to a710a43 Compare October 6, 2023 10:35

Manishearth reviewed Oct 6, 2023

View reviewed changes

python/servo/platform/base.py Outdated Show resolved Hide resolved

sagudev force-pushed the crown branch from e8904e6 to 093e29c Compare October 7, 2023 09:45

sagudev marked this pull request as ready for review October 7, 2023 10:31

mrobinson reviewed Oct 9, 2023

View reviewed changes

sagudev force-pushed the crown branch from 6bbc76e to 9cdfb32 Compare October 9, 2023 10:51

sagudev force-pushed the crown branch from ef30ac1 to ee215be Compare October 31, 2023 07:19

sagudev requested a review from mrobinson October 31, 2023 09:39

mrobinson reviewed Oct 31, 2023

View reviewed changes

components/script/lib.rs Outdated Show resolved Hide resolved

components/script/lib.rs Outdated Show resolved Hide resolved

python/servo/command_base.py Outdated Show resolved Hide resolved

sagudev force-pushed the crown branch from e72b365 to bce90ac Compare November 2, 2023 05:46

mrobinson changed the title ~~Replace script_plugins with crown (clippy like rustc driver)~~ Replace script_plugins with a clippy like rustc driver (named crown) Nov 17, 2023

mrobinson requested changes Nov 17, 2023

View reviewed changes

sagudev force-pushed the crown branch from 29c25ee to 984639f Compare November 17, 2023 09:47

sagudev force-pushed the crown branch from 47b237e to 124ad1d Compare November 17, 2023 10:48

mrobinson reviewed Nov 27, 2023

View reviewed changes

sagudev added 7 commits November 27, 2023 15:29

Remove script_plugins

5ed4f19

Use crown instead of script_plugins

7a8fc09

crown_is_not_used

11c4fd0

Use crown in command base

b514ac3

bootstrap crown

f26638c

tidy happy

0237fd6

disable sccache

9f38845

sagudev added the T-windows Do a try run on Windows label Dec 1, 2023

github-actions bot removed the T-windows Do a try run on Windows label Dec 1, 2023

sagudev added the T-windows Do a try run on Windows label Dec 1, 2023

github-actions bot removed the T-windows Do a try run on Windows label Dec 1, 2023

sagudev requested a review from mrobinson December 1, 2023 11:13

mrobinson reviewed Dec 1, 2023

View reviewed changes

servo-tidy.toml Outdated Show resolved Hide resolved

Ignore tidy on crown/tests

31656ea

sagudev force-pushed the crown branch from 6663feb to 31656ea Compare December 1, 2023 11:20

mrobinson approved these changes Dec 1, 2023

View reviewed changes

mrobinson added this pull request to the merge queue Dec 1, 2023

Merged via the queue into servo:master with commit 604979e Dec 1, 2023

mukilan mentioned this pull request Dec 2, 2023

./mach build broken on NixOS after moving to crown #30813

Closed

delan mentioned this pull request Dec 6, 2023

Windows try jobs fail when installing crown on older pull requests #30828

Closed

Mark-Simulacrum mentioned this pull request Jan 22, 2024

Alternatives to rustc_plugin::registry for (Servo’s) custom lints? rust-lang/rust#62868

Closed

weihanglo mentioned this pull request Feb 20, 2024

fix: remove unused sysroot_host_libdir rust-lang/cargo#13468

Merged

mukilan mentioned this pull request May 21, 2024

ci: enable sccache for Rust compilation. #32330

Merged

3 tasks

Uh oh!

Conversation

sagudev commented Oct 6, 2023 • edited by mrobinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagudev commented Oct 6, 2023

Uh oh!

Uh oh!

sagudev commented Oct 7, 2023

Uh oh!

github-actions bot commented Oct 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagudev commented Oct 10, 2023

Uh oh!

sagudev commented Oct 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sagudev commented Nov 2, 2023

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrobinson Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagudev Nov 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sagudev commented Nov 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 1, 2023

Uh oh!

github-actions bot commented Dec 1, 2023

Uh oh!

github-actions bot commented Dec 1, 2023

Uh oh!

github-actions bot commented Dec 1, 2023

Uh oh!

sagudev commented Dec 1, 2023

Uh oh!

Uh oh!

sagudev commented Oct 6, 2023 •

edited by mrobinson

Loading

mrobinson Nov 17, 2023 •

edited

Loading

sagudev Nov 17, 2023 •

edited

Loading