Don't modify PATH on OS X

[Arcanemagus]

There is a node executable in the apm/bin directory, which overrides any version of Node.js installed on the system since the PATH is being updated to put this path at the front. As the Linux side of the script does not do this, remove the modification of the PATH to stop breaking Node.js.

You can see a simplified version of the script being run here. This shows that Node.js is v6.2.2 at the start of the script, but after the PATH modification it shows Node.js at v0.10.40.

I'm not sure why this modification was there in the first place though, @joefitzgerald as you are the original author of that do you know if this is necessary?

[Arcanemagus]

This wasn't being used in the script, so I removed it as well.

[thomasjo]

ATOM_PATH is used by Atom though, specifically atom.sh.

[Arcanemagus]

Looks like I didn't explicitly mention this: This bug completely breaks linting with eslint@3 and any other linter requiring a Node.js newer than the one bundled with apm...

[thomasjo]

@atom/feedback This seems to work OK for normal packages. Are there any edge cases that I'm missing? Since this seems to be breaking CI for a lot of packages, I'm tempted to 🚢 this.

I'll wait a 12-ish hours unless I hear otherwise. After that I'm merging this 👶

[thomasjo]

I'm gonna merge this. If it breaks stuff, blame me 🔫

[thomasjo]

Turns out this won't fix any of the eslint problems since I'm assuming practically everyone has it listed as a dependency in their package's package.json, which means that it will be installed using apm which always uses its own Node, regardless of whatever else is installed on the system. That node is still v0.10.40.

One workaround for now is to not rely on the automatic linting support provided here, but rather to install eslint manually using npm, and also performing the linting manually. Sucks, but that's the reality.

The better workaround is to downgrade eslint of course, until atom/atom#11897 🚢s.

[joefitzgerald]

👎 on merging this; I think it should be reverted.

[thomasjo]

@joefitzgerald Why? We only set PATH for macOS, which seemed silly. We should do the same on all platforms at the very least.

[joefitzgerald]

Sure, let's do it for all of them. We should be using bundled node to do everything if possible. If people have issues with that, then - well - we need to upgrade bundled node (work in progress via @BinaryMuse) because it's what is being used anyway.

[thomasjo]

OK, as long as we have consistent behavior across all platforms, I agree that this can be reverted since it doesn't actually fix the intended problem.

[Arcanemagus]

I would be quite surprised if many projects (besides linter-eslint and fast-eslint and any others that actually use it in the package) use eslint as a dependency, and not just a dev dependency. It's quite common for a development environment to require things (Node.js v4+) no production environment (apm installs) supports.

How exactly do you propose we go about this then? Force everyone to wait on updating to eslint@3 for developing their projects until Atom stable is updated to a version of apm that has a new enough Node.js version to let everything work?

[joefitzgerald]

@Arcanemagus it's not clear to me that merging the PR actually resolved the issue for you. Thus, isn't this line of reasoning (that eslint 3.x is blocked on this PR) potentially missing the root cause of the eslint issue?

Regardless of dependency or devDependency, it is apm which is running apm install (which is implicitly npm install), and thus using its own embedded version of node. The fix is to get apm's node modernized, and that appears to be blocked on the (internal) build servers the atom team use to make release builds of atom. /cc @BinaryMuse

[Arcanemagus]

It had solved it for a few things, but others are broken due to the issue outlined in #43 (comment).

I think waiting on a stable release of Atom to include a fixed version of apm is a bit ridiculous myself, but I'm not sure on the best course of action to go forward.

My best idea that I've had so far is:

• Update the CI configurations to install Node.js v4 (latest LTS release)
• npm install using that release
• Run the linters
• Remove node_modules?
• Run apm install --production to run the install procedure through apm, ignoring any devDependencies that it may not be able to handle
• Run the specs

Do you see anything wrong with that, other than the slightly increased CI run times?

[Arcanemagus]

For anyone coming across this issue, I've implemented the above workaround over in https://github.com/Arcanemagus/ci/tree/atomlinter until an updated version of apm is included in a stable Atom release.