Skip to content

Fix running servoshell and unit tests through a symlink#30537

Merged
delan merged 5 commits intoservo:masterfrom
delan:fix-running-through-symlink
Oct 18, 2023
Merged

Fix running servoshell and unit tests through a symlink#30537
delan merged 5 commits intoservo:masterfrom
delan:fix-running-through-symlink

Conversation

@delan
Copy link
Member

@delan delan commented Oct 11, 2023

Servo relies on several external files in /resources/, and the loading of these resources is delegated to the embedder. In this repo, libsimpleservo compiles them in with include_bytes!(), while the other three (servoshell, gstplugin, and unit tests) try to read them from the filesystem at runtime.

Our current logic for searching for the resources directory tries to accommodate both local dev in this repo (../../resources relative to servo, or a further ancestor in the case of unit tests) and nightly builds (resources relative to servo, or Resources relative to a further ancestor in the case of macOS):

  1. let path = the current executable path
  2. canonicalise path, resolving any symlinks
  3. let path = parent of path
  4. try path/resources
  5. try path/Resources
  6. go to step 3

But this fails if target is symlinked to another filesystem, like I recently did to exclude it from my backups:

% ls -l target
lrwxrwxrwx 1 delan users 25 Sep 28 15:34 target -> /cuffs/build/servo/target

This patch adds (or replaces those with, for unit tests) a few steps as follows:

  1. let path = the current directory
  2. try path/resources
  3. let path = parent of path
  4. go to step 8

The new logic:

  • allows servoshell and unit tests to be run with a symlinked target directory
  • does not require any special environment variables, so you can run target/debug/servo in e.g. renderdoc
  • does not affect the behaviour of our nightly releases

  • There are tests for these changes OR
  • These changes do not require tests because ___

@delan delan requested a review from mukilan October 11, 2023 09:38
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Looks good with one small suggestion before landing.

@delan delan enabled auto-merge October 13, 2023 09:34
@delan delan added this pull request to the merge queue Oct 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 13, 2023
@delan
Copy link
Member Author

delan commented Oct 13, 2023

I had to rework the logic to fix breakages in unit tests and the macOS package smoketest, but as a result the changes are now less invasive and we no longer change the behaviour of nightly releases. #30509 will simplify this even further by baking in the resources for unit tests, it’s just we can’t do that in this patch.

@delan delan enabled auto-merge October 13, 2023 15:56
@delan delan added this pull request to the merge queue Oct 13, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 13, 2023
@delan delan added this pull request to the merge queue Oct 18, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 18, 2023
@delan delan added this pull request to the merge queue Oct 18, 2023
Merged via the queue into servo:master with commit 351b503 Oct 18, 2023
@delan delan deleted the fix-running-through-symlink branch October 18, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

servoshell and unit tests crash if target directory is symlinked

2 participants