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

src: define S_IWUSR & S_IRUSR for Windows by ilg-ul · Pull Request #42748 · nodejs/node · GitHub

/ node Public

src: define S_IWUSR & S_IRUSR for Windows - #42748

Closed
ilg-ul wants to merge 19 commits into
nodejs:masterfrom
xpack:S_IWUSR
ConversationCommits (19)ChecksFiles changed
Closed

src: define S_IWUSR & S_IRUSR for Windows#42748
ilg-ul wants to merge 19 commits into
nodejs:masterfrom
xpack:S_IWUSR

Conversation

ilg-ul commented Apr 15, 2022 •
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor

On Windows, most of the POSIX file mode definitions are not available.
However, functionally equivalent read/write definitions exists, and
chmod() can use them. This patch defines two aliases, so that these
definintions are issued in fs.constants.

Fixes: #41591

ilg-ul added 2 commits April 15, 2022 14:42
[#41591] define S_IWUSR & S_IRUSR for Windows
a5e1399
src: define S_IWUSR & S_IRUSR for Windows
62e3b8f 
On Windows, most of the POSIX file mode definitions are not available.
However, functionally equivalent read/write definitions exists, and
`chmod()` can use them. This patch defines two aliases, so that these
definintions are issued in `fs.constants`.

#41591
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 15, 2022
ilg-ul mentioned this pull request Apr 15, 2022
Closed

tniessen commented Apr 15, 2022

Copy link
Copy Markdown
Member

cc @nodejs/platform-windows @nodejs/libuv

ilg-ul commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

Please note that I do not have a Windows build environment available, and I could not check if this patch added the two definitions to fs.constants; this probably should be added to the tests, but I don't know how to do it.

ilg-ul added 10 commits April 15, 2022 18:38
src: define S_IWUSR & S_IRUSR for Windows
71fbff0 
On Windows, most of the POSIX file mode definitions are not available.
However, functionally equivalent read/write definitions exists, and
`chmod()` can use them. This patch defines two aliases, so that these
definintions are issued in `fs.constants`.

#41591
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
634f22e 
…WUSR
src: define fs.constants.S_IWUSR & S_IRUSR for Win
fbe9542 
On Windows, most of the POSIX file mode definitions are not available.
However, functionally equivalent read/write definitions exists, and
`chmod()` can use them. This patch defines two aliases, so that these
definintions are issued in `fs.constants`.

refs: #41591
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
980862f 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
a44c0ef 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
8443080 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
25ed12a 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
5dfde98 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
cf49526 
…WUSR
Merge branch 'S_IWUSR' of https://github.com/xpack/node-fork into S_I…
c20af35 
…WUSR
bnoordhuis approved these changes Apr 16, 2022
View reviewed changes

bnoordhuis left a comment

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

LGTM with some suggestions.

Comment thread test/parallel/test-fs-constants-windows.js Outdated
Comment on lines +8 to +9
assert.ok(fs.constants.S_IRUSR !== undefined);
assert.ok(fs.constants.S_IWUSR !== undefined);

bnoordhuis Apr 16, 2022

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

It'd be better to use assert.notStrictEqual() here. The "is windows?" guard is better written as:

if (!common.isWindows)
  common.skip('Windows-only test');

Although in its current incarnation it's not really Windows-specific. You could just remove the guard altogether and rename the file.

RaisinTen approved these changes Apr 16, 2022
View reviewed changes

RaisinTen left a comment

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

LGTM but could you please get rid of the merge commits? It negatively affects our tooling.

ilg-ul commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

please get rid of the merge commits?

Oops! I thought it is something wrong :-(

I'm using VS Code -> Amend. I should have used plain commit, right?

ilg-ul commented Apr 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

If the mess creates any problems, we can abandon this PR and I can try to create a new one, hopefully cleaner.

ilg-ul commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

Any idea how I managed to break the ASan test?

bnoordhuis commented Apr 16, 2022

Copy link
Copy Markdown
Member

That looks like a flaky test, not anything caused by this PR.

Apropos merge commits: git rebase origin master && git push git@github.com:xpack/node-fork.git +HEAD:S_IWUSR

ilg-ul commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

Recreated as #42757.

ilg-ul closed this Apr 16, 2022
ilg-ul deleted the S_IWUSR branch April 16, 2022 17:17
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

bnoordhuis bnoordhuis approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FS constants says not all constants are available on all platforms, but it doesn't say which ones are available on what platforms

5 participants

Back | FazBrowse Home | New Git URL