Summary

Fix Pipeline settings path validation for external node module paths.

Review Request #14816 — Created Feb. 12, 2026 and updated Feb. 27, 2026, 2:27 a.m.

Information

Owner

maubin

Repository

Djblets

Branch

release-5.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

We recently changed the way that we check whether babel, lessc, rollup
and uglifyjs binaries exist in the node modules path when building
Pipeline settings. We used to hard-code paths like
<node_modules_path>/.bin/rollup and check for their existence, but now
we use the npm exec command.

However, we invoke the command from the current working directory, when we
should actually be invoking it from the parent directory of the given node
modules path. This was made evident when trying to build Power Pack or run
its unit tests. Power Pack relies on the binaries in Review Board's node
modules, but npm exec was being run from Power Pack's node modules
directory.

This change fixes this so that the binaries are actually checked for in
the node modules paths, instead of the current working directory. And
we now include the consuming application's node modules path when
building the static media for an extension. If the binary is found,
then we use that path when running the binary with npm exec.

We also add handling for when node_modules_path is an empty string.
This can happen if we didn't find any node modules directories, which
should be an error state. When validate_paths=False and we have an
empty node_modules_path, we use the current working directory as
the path.

And finally this updates the docstring to (I hope) clarify the
validate_paths argument and when it should be set to False, and
clarifies the expectations of node_modules_path when
validate_paths=False.

Testing Done


Ran unit tests.
Was able to run Power Pack unit tests without hitting the

ImproperlyConfigured error for rollup.
Was able to build a Power Pack package against a development server

  of Review Board without hitting the ImproperlyConfigured error for

  rollup.
Set node_modules_path with validate_paths=True to an empty string,

  a real path valid path and a non-existent one separated by a colon,

  a real path valid path, and a non-existent path.
Ran the dev server, previously saw an error about the rollup command

  failing but its gone away with this fix.

Commits

Summary	ID
Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	e512d315f32d84a5e3a42055f4b8f084975efe9d

Summary

Fix Pipeline settings path validation for external node module paths.

We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.

e512d315f32d84a5e3a42055f4b8f084975efe9d

Issues

Description	From	Last Updated
Can we call this node_modules_paths?	david	Feb. 12, 2026, 9:04 p.m.
I tested here manually with a missing package and it prompted to install it. I checked the docs to see …	chipx86	Feb. 12, 2026, 7:25 p.m.
If any of the node_modules paths in the list lack this, will we get an error for each one until …	chipx86	Feb. 12, 2026, 8:48 p.m.
I wonder if we should now just log an error if a path in node_modules_path doesn't exist, since we now …	chipx86	Feb. 12, 2026, 8:48 p.m.
Can we have this be false, None (with equivalent typing change in the signature)? Alternatively we could just return str …	david	Feb. 18, 2026, 12:02 a.m.
typo: Its -> It's	david	Feb. 20, 2026, 1:33 a.m.
This doesn't need to be inside an else because of the exception.	david	Feb. 20, 2026, 1:35 a.m.
If validate_paths is False, we never populate exec_paths. Can you look at where we might be turning this off (either …	david	Feb. 27, 2026, 2:23 a.m.
We should probably run babel_path through shlex.quote() here.	david	Feb. 22, 2026, 8:51 p.m.
The babel implementation above is using .get(), which helps with the exec_paths population issue. This here will break for sure. …	david	Feb. 22, 2026, 10:14 p.m.
Same here.	david	Feb. 22, 2026, 10:14 p.m.
Same here.	david	Feb. 22, 2026, 10:14 p.m.
Same here.	david	Feb. 22, 2026, 10:14 p.m.

flake8 passed.

JSHint passed.

djblets/pipeline/settings.py (Diff revision 1)

The issue has been resolved. Show all issues

I tested here manually with a missing package and it prompted to install it. I checked the docs to see what the stated behavior was if automating this, and it turns out that it assumes it should install. Looks like we need to pass --no here to disable that.



If any requested packages are not present in the local project dependencies,

then a prompt is printed, which can be suppressed by providing either --yes

or --no. When standard input is not a TTY or a CI environment is detected,

--yes is assumed. The requested packages are installed to a folder in the npm

cache, which is added to the PATH environment variable in the executed

process.

maubin Feb. 12, 2026, 9:11 p.m.
```
Ah good catch, thanks.
```

djblets/pipeline/settings.py (Diff revision 1)

The issue has been dropped. Show all issues

If any of the node_modules paths in the list lack this, will we get an error for each one until found?

maubin Feb. 12, 2026, 9:11 p.m.

No, when a package isn't found the npm exec command doesn't raise an exception, it just has a non-zero return code. We'll only hit this exception case if executing the command failed in some way.

djblets/pipeline/settings.py (Diff revision 1)

The issue has been resolved. Show all issues

I wonder if we should now just log an error if a path in node_modules_path doesn't exist, since we now build a list of paths to try. We'd probably still need to bail if the resulting list of paths is empty.

maubin Feb. 12, 2026, 9:11 p.m.
```
That makes sense.
```

djblets/pipeline/settings.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we call this node_modules_paths?

maubin Feb. 12, 2026, 9:11 p.m.

Yeah, I was worried about it getting mixed up with node_modules_path but since they have different types it'll be evident if you use one instead of the other by mistake.

Change Summary:


Pass --no the npm exec to prevent it from auto installing missing packages.
Handles an empty node_modules_path.
Renamed node_modules_path_seq to node_modules_paths.
Include the node_modules_path in the error output for missing binaries since

  its useful.

Description:

		We recently changed the way that we check whether `babel`, `lessc`, `rollup`
		and `uglifyjs` binaries exist in the node modules path when building
		Pipeline settings. We used to hard-code paths like
		`<node_modules_path>/.bin/rollup` and check for their existence, but now
		we use the `npm exec` command.

		However, we invoke the command from the current working directory, when we
		should actually be invoking it from the parent directory of the node
		modules path.

		This was made evident when trying to run Power Pack's unit tests. Power Pack
		relies on Review Board's node modules, but since the `npm exec` command was
		ran from the Power Pack repository, it was looking in Power Pack's node
		modules instead of Review Board's.

		This change fixes this so that the binaries are actually checked for in
		the node modules paths, instead of the current working directory. If a
		binary isn't found in any of the configured node module paths then we
		raise the error.
	+
	+	Additionally, we now only raise an error if all configured node modules
	+	paths are invalid, whereas before we would raise an error if at least
	+	one is invalid even if there were valid ones.
	+
	+	This also adds handling for when `node_modules_path` is an empty string.
	+	This can happen if we didn't find any node modules directories. When
	+	building a `Path` object, an empty string is treated as the path to the
	+	current working directory. So before this change, the empty
	+	`node_modules_path` string would pass the path existence checks. Depending
	+	on the current working directory, the binaries existence checks could pass
	+	too. Now, we raise an error if `node_modules_path` is an empty string,
	+	because it should be treated as an error state instead of treating it as
	+	the path to the current working directory.

Testing Done:

		Ran unit tests.
		Was able to run Power Pack unit tests without hitting the `ImproperlyConfigured` error for rollup that I was hitting before.
	+	Set `node_modules_path` to an empty string, saw the logged error and improperly configured error raised.
	+	Set `node_modules_path` to a real path valid path and a non-existent one separated by a colon, saw the logged error for the non-existent path and the rest of the operation worked since the real path was valid.
	+	Set `node_modules_path` to a real path valid path and a trailing colon, meaning it would be split into the real path plus an empty string. Saw the logged error for the empty string and the rest of the operation worked since the real path was valid.
	+	Set `node_modules_path` to a non-existent one, saw the logged error and improperly configured error raised.

Commits:

	Summary	ID
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error.	fb589232b6e1785f61520dcfb59844fb9a73e2ba
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	0268383daf53ba74dc4b0f1f369cc046454e0f49

Diff:

Revision 2 (+86 -40)

Show changes

djblets/pipeline/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Change Summary:

Uses the node_modules path that the binary was found in when actually running the binary with npm exec.

Description:

		We recently changed the way that we check whether `babel`, `lessc`, `rollup`
		and `uglifyjs` binaries exist in the node modules path when building
		Pipeline settings. We used to hard-code paths like
		`<node_modules_path>/.bin/rollup` and check for their existence, but now
		we use the `npm exec` command.

		However, we invoke the command from the current working directory, when we
		should actually be invoking it from the parent directory of the node
		modules path.

		This was made evident when trying to run Power Pack's unit tests. Power Pack
		relies on Review Board's node modules, but since the `npm exec` command was
		ran from the Power Pack repository, it was looking in Power Pack's node
		modules instead of Review Board's.

		This change fixes this so that the binaries are actually checked for in
		the node modules paths, instead of the current working directory. If a
		binary isn't found in any of the configured node module paths then we
~		raise the error.
	~	raise the error. If found, then we use that path when running the
	+	binary with `npm exec`.

		Additionally, we now only raise an error if all configured node modules
		paths are invalid, whereas before we would raise an error if at least
		one is invalid even if there were valid ones.

		This also adds handling for when `node_modules_path` is an empty string.
		This can happen if we didn't find any node modules directories. When
		building a `Path` object, an empty string is treated as the path to the
		current working directory. So before this change, the empty
		`node_modules_path` string would pass the path existence checks. Depending
		on the current working directory, the binaries existence checks could pass
		too. Now, we raise an error if `node_modules_path` is an empty string,
		because it should be treated as an error state instead of treating it as
		the path to the current working directory.

Testing Done:

		Ran unit tests.
		Was able to run Power Pack unit tests without hitting the `ImproperlyConfigured` error for rollup that I was hitting before.
		Set `node_modules_path` to an empty string, saw the logged error and improperly configured error raised.
		Set `node_modules_path` to a real path valid path and a non-existent one separated by a colon, saw the logged error for the non-existent path and the rest of the operation worked since the real path was valid.
		Set `node_modules_path` to a real path valid path and a trailing colon, meaning it would be split into the real path plus an empty string. Saw the logged error for the empty string and the rest of the operation worked since the real path was valid.
		Set `node_modules_path` to a non-existent one, saw the logged error and improperly configured error raised.
	+	Ran the dev server, previously saw an error about the rollup command failing but its gone away with this fix.

Commits:

	Summary	ID
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	0268383daf53ba74dc4b0f1f369cc046454e0f49
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	b81dd55d67e29e5ad2fa66dbe44c5fa541f78ddf

Diff:

Revision 3 (+132 -50)

Show changes

djblets/pipeline/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/pipeline/settings.py (Diff revisions 2 - 3)

The issue has been resolved. Show all issues

Can we have this be false, None (with equivalent typing change in the signature)?
Alternatively we could just return str | None and have the type indicate success or failure directly.

Change Summary:


Respond to feedback.
Fix a problem with exec_paths being uninitialized when skipping path validation.

Commits:

	Summary	ID
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	b81dd55d67e29e5ad2fa66dbe44c5fa541f78ddf
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	1e1a139f60d77bb6518e3558566f5484e593e506

Diff:

Revision 4 (+148 -52)

Show changes

djblets/pipeline/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

djblets/pipeline/settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
typo: Its -> It's
```
djblets/pipeline/settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This doesn't need to be inside an else because of the exception.
```

djblets/pipeline/settings.py (Diff revision 4)

The issue has been resolved. Show all issues

If validate_paths is False, we never populate exec_paths. Can you look at where we might be turning this off (either via the arg or the environment variable) and see why we do that? I feel like we probably always want validation in our modern setups.

maubin Feb. 27, 2026, 2:27 a.m.

I'm going to leave the validate_paths scenarios as is because as discussed in Slack it's a bit of a rabbit hole. We currently have validate_paths=False when building packages against a production install of the consuming package, and when building static media (which happens when building a package against a dev install too). I was able to build a Power Pack package against a dev install of Review Board so I think this is ok.

djblets/pipeline/settings.py (Diff revision 4)

The issue has been resolved. Show all issues

We should probably run babel_path through shlex.quote() here.

maubin Feb. 27, 2026, 2:27 a.m.

Looks like these commands eventually get run through subproccess.Popen() with the default of shell=False, and I think shlex.quote() is only appropriate when we have shell=True.

We should actually be passing the command as a list of strings instead of one string, and then subprocess.Popen() will make sure everything's safe.

djblets/pipeline/settings.py (Diff revision 4)

The issue has been resolved. Show all issues

The babel implementation above is using .get(), which helps with the exec_paths population issue. This here will break for sure.

We should also quote the path.

djblets/pipeline/settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Same here.
```
djblets/pipeline/settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Same here.
```
djblets/pipeline/settings.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Same here.
```

Change Summary:


Always populate exec_paths and set the --prefix path for binaries, regardless of validate_paths.
Switch to setting a list of command args instead of a string for the <package>_BINARY commands.
Add some clarifications to the arg docs.
Include the consuming application's node_modules path when packaging static media for an extension.
Changed a missing node_modules path from an error to a warning, since it's not that severe (unless

  we're missing all node_modules paths, in which case we raise an error anyway).

Description:

		We recently changed the way that we check whether `babel`, `lessc`, `rollup`
		and `uglifyjs` binaries exist in the node modules path when building
		Pipeline settings. We used to hard-code paths like
		`<node_modules_path>/.bin/rollup` and check for their existence, but now
		we use the `npm exec` command.

		However, we invoke the command from the current working directory, when we
~		should actually be invoking it from the parent directory of the node
~		modules path.
~
~		This was made evident when trying to run Power Pack's unit tests. Power Pack
~		relies on Review Board's node modules, but since the `npm exec` command was
	~	should actually be invoking it from the parent directory of the given node
	~	modules path. This was made evident when trying to build Power Pack or run
	~	its unit tests. Power Pack relies on the binaries in Review Board's node
	~	modules, but `npm exec` was being run from Power Pack's node modules
	~	directory.
-		ran from the Power Pack repository, it was looking in Power Pack's node
-		modules instead of Review Board's.

		This change fixes this so that the binaries are actually checked for in
~		the node modules paths, instead of the current working directory. If a
~		binary isn't found in any of the configured node module paths then we
~		raise the error. If found, then we use that path when running the
~		binary with `npm exec`.
	~	the node modules paths, instead of the current working directory. And
	~	we now include the consuming application's node modules path when
	~	building the static media for an extension. If the binary is found,
	~	then we use that path when running the binary with `npm exec`.

~		Additionally, we now only raise an error if all configured node modules
~		paths are invalid, whereas before we would raise an error if at least
~		one is invalid even if there were valid ones.
	~	We also add handling for when `node_modules_path` is an empty string.
	~	This can happen if we didn't find any node modules directories, which
	~	should be an error state. When `validate_paths=False` and we have an
	+	empty `node_modules_path`, we use the current working directory as
	+	the path.

~		This also adds handling for when `node_modules_path` is an empty string.
~		This can happen if we didn't find any node modules directories. When
~		building a `Path` object, an empty string is treated as the path to the
~		current working directory. So before this change, the empty
	~	And finally this updates the docstring to (I hope) clarify the
	~	`validate_paths` argument and when it should be set to `False`, and
	~	clarifies the expectations of `node_modules_path` when
	~	`validate_paths=False`.
-		`node_modules_path` string would pass the path existence checks. Depending
-		on the current working directory, the binaries existence checks could pass
-		too. Now, we raise an error if `node_modules_path` is an empty string,
-		because it should be treated as an error state instead of treating it as
-		the path to the current working directory.

Testing Done:

		Ran unit tests.
~		Was able to run Power Pack unit tests without hitting the `ImproperlyConfigured` error for rollup that I was hitting before.
~		Set `node_modules_path` to an empty string, saw the logged error and improperly configured error raised.
~		Set `node_modules_path` to a real path valid path and a non-existent one separated by a colon, saw the logged error for the non-existent path and the rest of the operation worked since the real path was valid.
~		Set `node_modules_path` to a real path valid path and a trailing colon, meaning it would be split into the real path plus an empty string. Saw the logged error for the empty string and the rest of the operation worked since the real path was valid.
	~	Was able to run Power Pack unit tests without hitting the `ImproperlyConfigured` error for rollup.
	~	Was able to build a Power Pack package against a development server of Review Board without hitting the `ImproperlyConfigured` error for rollup.
	~	Set `node_modules_path` with `validate_paths=True` to an empty string, a real path valid path and a non-existent one separated by a colon, a real path valid path, and a non-existent path.
	~	Ran the dev server, previously saw an error about the rollup command failing but its gone away with this fix.
-		Set `node_modules_path` to a non-existent one, saw the logged error and improperly configured error raised.
-		Ran the dev server, previously saw an error about the rollup command failing but its gone away with this fix.

Commits:

	Summary	ID
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	1e1a139f60d77bb6518e3558566f5484e593e506
	Fix Pipeline settings path validation for external node module paths. We recently changed the way that we check whether `babel`, `lessc`, `rollup` and `uglifyjs` binaries exist in the node modules path when building Pipeline settings. We used to hard-code paths like `<node_modules_path>/.bin/rollup` and check for their existence, but now we use the `npm exec` command. However, we invoke the command from the current working directory, when we should actually be invoking it from the parent directory of the node modules path. This was made evident when trying to run Power Pack's unit tests. Power Pack relies on Review Board's node modules, but since the `npm exec` command was ran from the Power Pack repository, it was looking in Power Pack's node modules instead of Review Board's. This change fixes this so that the binaries are actually checked for in the node modules paths, instead of the current working directory. If a binary isn't found in any of the configured node module paths then we raise the error. If found, then we use that path when running the binary with `npm exec`. Additionally, we now only raise an error if all configured node modules paths are invalid, whereas before we would raise an error if at least one is invalid even if there were valid ones. This also adds handling for when `node_modules_path` is an empty string. This can happen if we didn't find any node modules directories. When building a `Path` object, an empty string is treated as the path to the current working directory. So before this change, the empty `node_modules_path` string would pass the path existence checks. Depending on the current working directory, the binaries existence checks could pass too. Now, we raise an error if `node_modules_path` is an empty string, because it should be treated as an error state instead of treating it as the path to the current working directory.	e512d315f32d84a5e3a42055f4b8f084975efe9d

Diff:

Revision 5 (+248 -80)

Show changes

	djblets/extensions/packaging/static_media.py
	djblets/pipeline/settings.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.