lib: fix module linter issues by setting root

[gibfahn]

Eslint searches parent directories looking for config files until it
finds one with "root: true" in it. This sets "root: true" in CitGM's
tmpDir to avoid that.

Fixes: #399

Checklist

• npm test passes
• tests are included
• contribution guidelines followed here

To test:

mkdir /path/to/node/citgm_tmp
node /path/to/citgm.js --tmpDir /path/to/node/citgm_tmp --v silly gulp

[gdams]

@nodejs-github-bot run ci

[nodejs-github-bot]

@gdams build started: https://ci.nodejs.org/job/citgm-continuous-integration-pipeline/

[gibfahn]

I've run this manually and it seems to resolve the issue (citgm gulp fails without this, passes with it).

Needs a test.

[gibfahn]

@nodejs-github-bot run CI

EDIT: CI was green

[nodejs-github-bot]

@gibfahn build started: https://ci.nodejs.org/job/citgm-continuous-integration-pipeline/

[gibfahn]

@nodejs-github-bot run CI

[nodejs-github-bot]

@gibfahn build started: https://ci.nodejs.org/job/citgm-continuous-integration-pipeline/

[MylesBorins]

So quick question. Should we be floating fixes for specific oddities in a package? Eslint is obviously widely adopted, but I'm not sure if like the precident this sets. Why did this change? Is it an upstream change? Is it due to the use of yaml? Is it something that can be fixed upstream? Seems to me that this would be a big that would affect other projects potentially. Could we fix this in CI? I'm not going to be stubborn and keep our CI broken on principle, but want to ensure that we are not just creating a bandaid... Citgm breaking here may be a bigger code smell

…

On Apr 25, 2017 11:39 AM, "George Adams" ***@***.***> wrote: ***@***.**** approved this pull request. LGTM — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#400 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV_sCKajBQBXdnw55JTx_zTFFo9vZks5rzhPCgaJpZM4NHXfM> .

[Trott]

Why did this change? Is it an upstream change? Is it due to the use of
yaml? Is it something that can be fixed upstream? Seems to me that this
would be a big that would affect other projects potentially.

• I don't think anything changed on the ESLint side with the way cascading config files work, but /cc @not-an-aardvark just in case I'm wrong about that.

• The directory structure in CITGM didn't change for CI runs, did it? Because it seems like it has to be that or else the bullet point above....

[not-an-aardvark]

I don't think anything changed on the ESLint side with the way cascading config files work, but /cc @not-an-aardvark just in case I'm wrong about that.

No, I don't think anything changed about that recently.

[MylesBorins]

So it seems to me like this could be solved in CI by simply not making the temp directory inside of the smoker directory where node is cloned. I'm still very weirded out by things suddenly braking though, and I'm going to see if I can chase it down to a specific change in eslint

[gibfahn]

@not-an-aardvark is there a reason for a module not to set root: true in their eslint config? I'm not sure why that isn't the default in eslint.

We should definitely clone node into $WORKSPACE/node though, that's probably a more sensible thing to do anyway.

[not-an-aardvark]

For a config file at a project root, setting root: true is a good idea. However, for a config file in a subfolder (e.g. our test/eslintrc.yaml file), setting root: true would not be a good idea because that config is intended to inherit from the config in the root directory.

It's possible that root: true would be a better default (I wasn't working on the project when that decision was made) but for now all I can say is that the default behavior is probably not going to change to root: true in the near future because that would cause a lot of breakage.

[gibfahn]

@not-an-aardvark yeah, that's what I thought, thanks. Agreed that there's no point changing the default now, I was just checking that I wasn't missing some obvious reason for a module like gulp not to set that option before I PR its addition...

So PRs to the relevant modules to add this would make sense? I can do that.

[MylesBorins]

So the problem repos are extending https://github.com/gulpjs/eslint-config-gulp

perhaps that should get a root: true?

This seems really fragile to me. I'm so bewildered about how this is just popping up. Nothing in any of these packages seems to have changed any time recently

[MylesBorins]

As much as it bothers me I think we should just land this and cut a release so we can finish the tests

[gibfahn]

@MylesBorins it shouldn't be too difficult to just add in the option to the git plugin to clone node into node/ (famous last words)...

Update: I've made the changes to citgm-smoker, it seems to work, but there is an issue where any machine that has the old workspace fails the git clone step (can't create folder node/ because there's already a node symlink in the workspace root). The fix is to clean that workspace and try again.

This is a pain, the good thing is that this only has to be done once per machine, and that it fails immediately, so you can just check after 10s and then rerun. I've run it a bunch of times and I hope that I've cleaned them all up, but I'm not sure there's a good way to check.

To fix:

1. Click on the failing test:

2. Remove the build number from the URL:
   https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/739/nodes=ubuntu1204-64/ ->
   https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/nodes=ubuntu1204-64/

3. Click Workspace

4. Click Wipe Out Current Workspace

5. Abort the run and rebuild.

[MylesBorins]

@gibfahn would you be able to go in and fix the abi smoker too?

[gibfahn]

@gibfahn would you be able to go in and fix the abi smoker too?

I think it's fixed now, I set the npm_config_nodedir environment variable to work around #401. We should take a look at the failures in the latest run and see whether anything looks like a script issue.

[gibfahn]

Closing in favour of gulpjs/eslint-config-gulp#12 and the Jenkins script fixes.