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

Bumped nsfw to 1.2.9 #7535

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Conversation

federicobozzini
Copy link
Contributor

What it does

It bumps nsfw version to 1.2.9.

This fixes #6315 . It should also help with #7510 since the napi changes that were breaking the build are not included in this release.

How to test

As for #6315 just moving a file on Windows should now fire the correct events.

Review checklist

Reminder for reviewers

Signed-off-by: Federico Bozzini <federico.bozzini@gmail.com>
@akosyakov akosyakov added the filesystem issues related to the filesystem label Apr 9, 2020
Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I could build it:

Done in 5.46s.
Done in 493.31s.
yarn run v1.21.1
$ theia rebuild:electron
Processing @theia/node-pty
Processing nsfw
Processing native-keymap
Processing find-git-repositories
√ Rebuild Complete
Done in 51.20s.

I could rebuild for electron:

yarn run v1.21.1
$ theia rebuild:electron
native node modules are already rebuilt for electron
Done in 3.06s.

The versions look good:

yarn why v1.21.1
[1/4] Why do we have the module "nsfw"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "nsfw@1.2.9"
info Reasons this module exists
   - "_project_#@theia#core" depends on it
   - Hoisted from "_project_#@theia#core#nsfw"
Done in 1.33s.

File change event looks OK on Windows after moving a single file:
Annotation 2020-04-09 095830

👍

@marcdumais-work
Copy link
Contributor

Hi,

I am unable to reproduce on Ubuntu 18.04, using the suggested theia-apps application. I notice that it's already pulling nsfw@1.2.9

Probably doesn't hurt to bump the nsfw dependency version though. I do not have a Windows machine to test the bug specific to that platform.

@marcdumais-work
Copy link
Contributor

@federicobozzini have you gone through our 3rd party dependencies license check process to confirm that everything recursively pulled along the new version of nsfw is still ok for the Foundation, license-wise?

If not can you give it a try? I can help if you have questions.

@benoitf
Copy link
Contributor

benoitf commented Apr 9, 2020

by default it's specified ^1.2.2 so it will resolve to 1.2.9 if you drop yarn.lock

@federicobozzini
Copy link
Contributor Author

@marcdumais-work

@federicobozzini have you gone through our 3rd party dependencies license check process to confirm that everything recursively pulled along the new version of nsfw is still ok for the Foundation, license-wise?

There are no new dependency as far as I can tell. With this PR nsfw version bumps to 1.2.9. and as a consequence a few unused recursive dependencies are dropped. That's all.

@marcdumais-work
Copy link
Contributor

There are no new dependency as far as I can tell. With this PR nsfw version bumps to 1.2.9. and as a consequence a few unused recursive dependencies are dropped. That's all.

A new version of any production dependency is enough to trigger the need for the license check

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Apr 9, 2020

There are no new dependency as far as I can tell. With this PR nsfw version bumps to 1.2.9. and as a consequence a few unused recursive dependencies are dropped. That's all.

A new version of any production dependency is enough to trigger the need for the license check

I quickly verified the updated dependency by performing a license check (as described here) and everything checks out :)

@marcdumais-work
Copy link
Contributor

by default it's specified ^1.2.2 so it will resolve to 1.2.9 if you drop yarn.lock

So, just playing devil's advocate: since a new ok v1.2.x that works has been released, that will be picked-up by clients with no yarn.lock, do we still need this PR?

@akosyakov
Copy link
Member

So, just playing devil's advocate: since a new ok v1.2.x that works has been released, that will be picked-up by clients with no yarn.lock, do we still need this PR?

Yes, we need to test the latest nsfw by us.

@akosyakov
Copy link
Member

merging or anything else has to be resovled?

@marcdumais-work
Copy link
Contributor

merging or anything else has to be resovled?

Good from my side 👍

@akosyakov akosyakov merged commit acfc086 into eclipse-theia:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fylesystem] Renaming a file on windows generates the wrong event
6 participants