Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: define S_IWUSR & S_IRUSR for Windows #42748

Closed
wants to merge 19 commits into from
Closed

Conversation

ilg-ul
Copy link
Contributor

@ilg-ul ilg-ul commented Apr 15, 2022

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
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 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
@tniessen
Copy link
Member

cc @nodejs/platform-windows @nodejs/libuv

@ilg-ul
Copy link
Contributor Author

ilg-ul commented Apr 15, 2022

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
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
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
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with some suggestions.

Comment on lines 8 to 9
assert.ok(fs.constants.S_IRUSR !== undefined);
assert.ok(fs.constants.S_IWUSR !== undefined);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

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

@ilg-ul
Copy link
Contributor Author

ilg-ul commented Apr 16, 2022

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
Copy link
Contributor Author

ilg-ul commented Apr 16, 2022

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

@ilg-ul
Copy link
Contributor Author

ilg-ul commented Apr 16, 2022

Any idea how I managed to break the ASan test?

@bnoordhuis
Copy link
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
Copy link
Contributor Author

ilg-ul commented Apr 16, 2022

Recreated as #42757.

@ilg-ul ilg-ul closed this Apr 16, 2022
@ilg-ul ilg-ul deleted the S_IWUSR branch April 16, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
5 participants