migrate eslint (mostly) from local packages to vendored node_modules

NEW
Assigned to

Status

task
P2
normal
7 months ago
19 days ago

People

(Reporter: dmose, Assigned: standard8)

Tracking

(Blocks 5 bugs)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [blocked on node modules security sign-off])

Attachments

(4 attachments)

(Reporter)

Description

7 months ago
Since eslint currently expects to use/control the node_modules space, it needs to be the first package migrated to the new node_modules structure.  

http://bit.ly/2x6gcaw has some basic notes.

Standard8, :ryanvm wondered if it would be possible to break out the work that makes eslint use the NODEJS found by configure and land that first (maybe as a dependent bug), so that he can remove NodeJS from MozillaBuild (bug remove-mozilla-build-node).  What are your thoughts?
(Reporter)

Updated

7 months ago
Blocks: 1484235
(Reporter)

Updated

7 months ago
Blocks: 1484236
No longer blocks: 1484235
(Reporter)

Updated

7 months ago
Blocks: remove-mozilla-build-node
No longer blocks: 1484236
(Assignee)

Comment 1

7 months ago
(In reply to Dan Mosedale (:dmose, :dmosedale) from comment #0)
> Standard8, :ryanvm wondered if it would be possible to break out the work
> that makes eslint use the NODEJS found by configure and land that first
> (maybe as a dependent bug), so that he can remove NodeJS from MozillaBuild
> (bug remove-mozilla-build-node).  What are your thoughts?

Discussed on irc and decided bug 1482435 would be the one to that work.
Depends on: 1482435
(Assignee)

Comment 2

7 months ago
I think the biggest question here, is do we have sufficient cross-platform & vcs support for symlinks in node_modules?

I can see two options: 1) check in the symlinks to source control. 2) Manually construct the symlinks when we need them (e.g. ESLint's setup routine).

Gergory, do you have any recommendations here?

Currently in an installed node_modules directory we have:

- symlinks in .bin/, e.g:

node_modules/.bin/eslint -> ../eslint/bin/eslint.js

We don't actually use this (we call the eslint.js script direct via node), but we might want to use similar links for other node/npm commands I guess?

- symlinks for the eslint-plugins, i.e.:

node_modules/eslint-plugin-mozilla -> ../tools/lint/eslint/eslint-plugin-mozilla
node_modules/eslint-plugin-spidermonkey-js -> ../tools/lint/eslint/eslint-plugin-spidermonkey-js

These are installed as a result of the file:/ urls. I think if we're vendoring node_modules, then we don't really want to move the sources for those into node_modules - though I'm welcome to opinions here.
Flags: needinfo?(gps)
(Assignee)

Comment 3

7 months ago
I think the hg hooks have just answered the symlink question this for me:

remote: ****************************** ERROR *******************************
remote: 3ae69eef7bad adds or modifies the following symlinks:
remote: 
remote:   node_modules/.bin/acorn
...
remote: Symlinks aren't allowed in this repo. Convert these paths to regular
remote: files and try your push again.
remote: ********************************************************************

So I think we'll still need ESLint to add the symlinks for eslint-plugin-* directories, unless there's better ideas.
(Assignee)

Comment 4

7 months ago
Experimental try push with symlinks created via support scripts:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=423f8f2302db75f5d478e9e5376ef6a8caa284b4

I think this is probably going to be the way we need to go with this.
(Assignee)

Updated

7 months ago
Depends on: 1495397
(Assignee)

Updated

7 months ago
Blocks: 1493610
(Reporter)

Comment 5

7 months ago
(In reply to Mark Banner (:standard8) from comment #3)
> I think the hg hooks have just answered the symlink question this for me:
> 
> remote: ****************************** ERROR *******************************
> remote: 3ae69eef7bad adds or modifies the following symlinks:
> remote: 
> remote:   node_modules/.bin/acorn
> ...
> remote: Symlinks aren't allowed in this repo. Convert these paths to regular
> remote: files and try your push again.
> remote: ********************************************************************
> 
> So I think we'll still need ESLint to add the symlinks for eslint-plugin-*
> directories, unless there's better ideas.

It would be interesting to know the rationale for this.  It's possible, after all, that the economics around this decision have changed since it was made, and that maybe it's worth undoing.  gps, it would be helpful to know your thoughts here...
The commit message for https://hg.mozilla.org/hgcustom/version-control-tools/rev/02675c286d0f explains the rationale.

I *really* don't want to open the flood gates and allow symlinks in the repo. We'll just be opening ourselves up to a lot of pain from Windows users not committing the right data into version control.
Flags: needinfo?(gps)
(Assignee)

Updated

6 months ago
Blocks: 1498604
(Reporter)

Comment 12

6 months ago
Mark, do you have another link to that try push :-) ?
(Reporter)

Updated

6 months ago
This is blocking another bug, is this a P1 or P2 now?
(Assignee)

Comment 15

4 months ago
At the moment, this is blocked by non-bug work for getting approvals for having node_modules vendored in tree and sign-off on how we review security etc to make that happen.

Once that's done this is probably a P1, but I'll stick it as P2 for now.
Priority: -- → P2
Whiteboard: [blocked on node modules security sign-off]
Component: General → Lint and Formatting
(Assignee)

Updated

19 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.