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

Replace nsfw with @parcel/watcher #12784

Closed
wants to merge 3 commits into from
Closed

Conversation

msujew
Copy link
Member

@msujew msujew commented Jul 31, 2023

What it does

Closes #12782.

Replaces our nsfw dependency with @parcel/watcher to align with vscode and to reduce our reliance on new libc versions.

How to test

  1. Test file watching related capabilities and confirm that there are no regressions:
    • Editing files on the file system should be reflected in the editor
    • Editing settings.json should be reflected in changes in the preferences
    • Editing launch.json should be reflected in the debug view

Review checklist

Reminder for reviewers

@msujew msujew added filesystem issues related to the filesystem dependencies pull requests that update a dependency file file-watchers issues related to filesystem watchers - nsfw labels Jul 31, 2023
@tsmaeder
Copy link
Contributor

@msujew do you feel this is pretty much a 1:1 replacement? Are we aware what the advantages/disadvantages of this change is? How does it affect adopters? We're exporting it from core/shared, after all.

@msujew
Copy link
Member Author

msujew commented Aug 11, 2023

@tsmaeder Yes, this is supposed a 1:1 replacement without any functional advantages/disadvantages. The only drawback that I've noticed is that you cannot watch single files anymore. Only directories.

Any adopter that has previously been using NSFW in their code will need to switch to the parcel watcher. The API is pretty similar, so hopefully the migration shouldn't be too difficult for adopters.

The main motivation behind this change is to move to native dependencies that don't rely so heavily on new versions of libc on Linux. See arduino/arduino-ide#2018 and #12782 (with #12618 being closely related).

@marcdumais-work
Copy link
Contributor

marcdumais-work commented Aug 17, 2023

@msujew I like the idea!

One potential concern is that it might do not as well, in a Linux environment where the inotify resource is near depletion [1]. I expect this would mostly be an issue with multi-user machines running Linux. Single user machines should be ok, so long as the number of inotify is increased from default (at least on Ubuntu, default is really low).

The only drawback that I've noticed is that you cannot watch single files anymore. Only directories

I think previously we would immediately, on startup, individually watch specific, important config files, like settings.json and others, and also any file opened in the editor. IIRC, NSFW on Linux would watch single files by polling (reserving inotify for directories). This way of watching individual files could work even in an environment depleted of inotify resource. It's been a while since I did a deep-dive into that code, so I may misremember or be unaware of later changes.

Have you noticed anything in vscode's side, along with the switch to @parcel/watcher, that would potentially mitigate such issues? Maybe this would be a "Cloud-related" optimisation, not deemed too important for code-oss (wink wink)?

Would you please rebase this PR to latest master?

[1]: e.g. Imagine a service that offers "push the button" containerized development environments, that each include a Theia app. The host runs Linux, and the inotify limit is shared among all containers running on it. All it takes is a few huge workspaces / repos (in terms of the number of contained directories) being opened on a given host, to suck-up most inotify, and make it more likely that a newly opened workspace, might run-out to some extent, intermittently.

update: In other words, if I'm correct about how it works with NSFW on Linux, I think we would always have reliable "watches" on a small set of vital files, whose watching is required for the IDE to minimally work. I wonder if @parcel/watcher can do similar? Or maybe there exists a polling-based watcher we could use on the side, for individual files?

@marcdumais-work
Copy link
Contributor

Looking quickly at the @parcel/watcher bug tracker, some of of the issues give me a bit of pause.

parcel-bundler/watcher#122
parcel-bundler/watcher#97

This reminds me, we should test successful watching through symbolic links

@msujew
Copy link
Member Author

msujew commented Aug 18, 2023

I've dug a bit deeper into the glibc rabbit hole in relation with nsfw. Now things a bit more clear to me in regards to why the binaries I distribute over here even require such a new version of glibc. I've downgraded the required glibc version, which makes this kind of obsolete.

I'll close this PR, but in case anyone wants to come back to this topic, you can see the required changes in the changes tab of this PR.

@marcdumais-work
Copy link
Contributor

That's awesome, thanks @msujew

@msujew msujew mentioned this pull request Aug 6, 2024
@msujew msujew mentioned this pull request Sep 18, 2024
1 task
@tsmaeder
Copy link
Contributor

I've looked into the failures of the Playwright tests. They only fail on Linux. IMO, this is a bug in the "compressed tree" and is probably unrelated to the change at hand. So, with the resources folder from the playwright test, you can do the following scenario:

  1. delete "nestedFolder2"
  2. Observe: there is no change visible and a console.log in the rendering of the compressed rows is not hit.
  3. Select the row in question
  4. Observe: nestedFolder2 disappears from the row.

The same events seem to be sent on Linux and Windows, so my suspicion is that this is a reordering of async blocks du to the different timings on the respective platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file file-watchers issues related to filesystem watchers - nsfw filesystem issues related to the filesystem
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Evaluate alternatives for native dependencies
3 participants