Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fixes for building when the path to the repo contains spaces - #56401

Closed
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces
Closed

Fixes for building when the path to the repo contains spaces#56401
Jayman2000 wants to merge 2 commits into
nodejs:mainfrom
Jayman2000:build-fixes-for-paths-with-spaces

Conversation

@Jayman2000

@Jayman2000Jayman2000 commented Dec 29, 2024

Copy link
Copy Markdown

The “Building Node.js” guide says:

If the path to your build directory contains a space, the build will likely
fail.

This pull request contains a few fixes that are directed at that problem. I still haven’t been able to successfully build and test Node.js while it’s stored in a path that contains spaces, but the changes in this pull request have gotten me closer to being able to do that.

Unfortunately, in order to test one of the commits in this pull request, a vendored dependency of a vendored dependency of a vendored dependency needs to be patched. I’m going to mark this pull request as a draft until the following has happened:

  • I have submitted a nodejs/gyp-next pull request with the required changes.
  • That pull request gets merged.
  • There’s a new release of nodejs/gyp-next that contains the changes from that pull request.
  • Someone submits a nodejs/node-gyp pull request that updates nodejs/node-gyp’s version of nodejs/gyp-next.
  • That pull request gets merged.
  • There’s a new release of nodejs/node-gyp that contains the new version of nodejs/gyp-next.
  • Someone submits an npm/cli pull request that updates npm/cli’s version of nodejs/node-gyp.
  • That pull request gets merged.
  • There’s a new release of npm/cli that contains the new version of nodejs/node-gyp.
  • Someone submits a nodejs/node pull request that updates nodejs/node’s version of npm/cli.
  • That pull request gets merged.
  • I rebase this current pull request on nodejs/node’s main branch.
  • I add testing instructions to this current pull request.

Before this change, Makefile would sometimes use quotation marks when
expanding the NODE Make variable and sometimes would not use quotation
marks when expanding the NODE Make variable. This change makes it so
that the Makefile never uses quotation marks when expanding the NODE
variable.
Line 82 of the Makefile sets the NODE variable to this:
NODE ?= "$(PWD)/$(NODE_EXE)"
Since there are already quotation marks embedded in the value of the
NODE variable, we don’t need to add additional quotes when expanding the
variable. In fact, adding quotes can cause issues. For example, before
this change, line 97 of the Makefile said this:
if [ -x "$(NODE)" ] && [ -e "$(NODE)" ]; then \
If you tried to run “make test-only” and the path your copy of the
Node.js source code contained spaces, then that line would expand to
something like this:
if [ -x ""/Path that contains spaces/node"" ] && [ -e ""/Path that contains spaces/node"" ]; then \
The shell would then fail to run that command because of the improper
quoting.
The Makefile contains two variable: NODE and run-npm-ci. Here’s the line
that sets the NODE variable:
NODE ?= "$(PWD)/$(NODE_EXE)"
The value of the NODE variable contains quotation marks just in case
$(PWD) contains spaces. Before this change, here’s how the run-npm-ci
variable was set:
run-npm-ci = $(PWD)/$(NPM) ci
The value of the run-npm-ci variable does not contain quotation marks.
$(run-npm-ci) is supposed expand into two words when interpreted by a
shell, but if $(PWD) contained spaces, then it would expand into more
than 2 words.
This change adds quotation marks to the value of run-npm-ci. This change
makes the Makefile more consistent and helps make sure that
$(run-npm-ci) does the right thing, even if $(PWD) contains spaces.
@nodejs-github-botnodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Dec 29, 2024
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actionsgithub-actionsBot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@Jayman2000

Copy link
Copy Markdown
Author

Now that #60511 has been merged, this pull request is obsolete.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to Node.js builds or CI infrastructure.needs-ciPRs that need a full CI run.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Jayman2000@nodejs-github-bot