FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: make icu download path customisable by jbergstroem · Pull Request #3200 · nodejs/node · GitHub

/ node Public

build: make icu download path customisable - #3200

Merged
jbergstroem merged 1 commit into
nodejs:masterfrom
jbergstroem:feature/icu-download-path
Oct 8, 2015
ConversationCommits (1)ChecksFiles changed
Merged

build: make icu download path customisable#3200
jbergstroem merged 1 commit into
nodejs:masterfrom
jbergstroem:feature/icu-download-path

Conversation

jbergstroem commented Oct 6, 2015

Copy link
Copy Markdown
Member

This makes it easier to store icu tarballs outside of the node.js directory which is useful in our CI where git directories are scrubbed between runs.

/R=@srl295, @rvagg

jbergstroem added the build Issues and PRs related to build files or the CI. label Oct 6, 2015
mscdex added the i18n-api Issues and PRs related to the i18n implementation. label Oct 6, 2015

srl295 commented Oct 6, 2015 via email

Copy link
Copy Markdown
Member

bnoordhuis reviewed Oct 6, 2015
View reviewed changes
Comment thread configure Outdated

bnoordhuis Oct 6, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Something of an anti-pattern, this. Whether the path exists doesn't say anything about it being accessible.

jbergstroem Oct 6, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

use os.access instead?

srl295 mentioned this pull request Oct 6, 2015
Closed

rvagg commented Oct 7, 2015

Copy link
Copy Markdown
Member

lgtm!

jasnell commented Oct 7, 2015

Copy link
Copy Markdown
Member

LGTM

jbergstroem force-pushed the feature/icu-download-path branch from 6c94583 to 92c7a6f Compare October 8, 2015 00:49

jbergstroem commented Oct 8, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis I slightly tweaked the error message and now check for writability through os.access.

bnoordhuis commented Oct 8, 2015

Copy link
Copy Markdown
Member

Better, I suppose. LGTM.

build: make icu download path customizable
a214905 
This makes it easier to store icu tarballs outside of the node.js
directory which is useful in our CI where git directories are
scrubbed between runs.

PR-URL: nodejs#3200
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
jbergstroem force-pushed the feature/icu-download-path branch from 92c7a6f to a214905 Compare October 8, 2015 11:40
jbergstroem merged commit a214905 into nodejs:master Oct 8, 2015

jbergstroem commented Oct 8, 2015

Copy link
Copy Markdown
Member Author

Just for the record:

[22:02:04]  <jbergstroem>   srl295: changes in https://github.com/nodejs/node/pull/3200 still look ok?
[22:03:52]  <jbergstroem>   rvagg: ^
[22:39:06]  <rvagg> jbergstroem: do it

jbergstroem added a commit that referenced this pull request Oct 8, 2015
build: make icu download path customizable
9136359 
This makes it easier to store icu tarballs outside of the node.js
directory which is useful in our CI where git directories are
scrubbed between runs.

PR-URL: #3200
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
jasnell mentioned this pull request Oct 8, 2015
Closed

MylesBorins commented Nov 17, 2015

Copy link
Copy Markdown
Contributor

landed in v4.x-staging in 9136359

snyk-io Bot mentioned this pull request Oct 23, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. i18n-api Issues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL