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.

Sign up for GitHub

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

Jump to bottom

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

Closed
dead-claudia opened this issue Jan 19, 2022 · 11 comments
Closed

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

dead-claudia opened this issue Jan 19, 2022 · 11 comments
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.

Comments

@dead-claudia
Copy link

Affected URL(s)

https://nodejs.org/api/fs.html#fsconstants

Description of the problem

It says the following:

Not every constant will be available on every operating system.

However, this doesn't say anything else about the availability of various constants. In particular, none of the file mode constants are available on Windows and about half of the file type constants are also not present, but the docs don't even so much as hint at this. (I initially thought this to be a bug and filed #41590, so it's definitely confusing.) Part of what makes it non-obvious is the fact many of the various Unix-based constants (both in fs and elsewhere like with many signals) are shimmed on Windows in terms of corresponding Windows APIs, and so I can't just assume for all of them. (In particular, fs.constants.O_DIRECTORY is missing from Windows despite fs.opendir using Windows APIs similar to POSIX's opendir - that one was very surprising.)

Could the series of tables be updated to include platform availability for each relevant constant?
@dead-claudia dead-claudia added the doc Issues and PRs related to the documentations. label Jan 19, 2022
@Mesteery Mesteery added the fs Issues and PRs related to the fs subsystem / file system. label Jan 19, 2022
@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 14, 2022

I've also got bitten by the missing S_IWUSR on Windows.

I think that this particular constant must be defined for Windows, since chmod() is able to set this bit; perhaps other constants are in this category too.

The constants not used on Windows can be omitted, but the documentation must explicitly mention this.

@bnoordhuis bnoordhuis added the good first issue Issues that are suitable for first-time contributors. label Apr 15, 2022
@bnoordhuis
Copy link
Member

I've added the "good first issue" label because it just needs someone to go and open a pull request.

@ilg-ul S_IWUSR is missing on Windows because it's called _S_IWRITE there (which node doesn't export.)

You could open a pull request exporting _S_IWRITE and _S_IREAD, or aliasing them to S_IWUSR and S_IRUSR.

@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 15, 2022

I took a look, and I identified in node_constants.cc several definitions that add constants. Is this the correct location?

If so, I suggest we alias the definitions like this:

#if defined(_WIN32)
#include <io.h>  // _S_IREAD _S_IWRITE
#ifndef S_IRUSR
#define S_IRUSR _S_IREAD
#endif  // S_IRUSR
#ifndef S_IWUSR
#define S_IWUSR _S_IWRITE
#endif  // S_IWUSR
#endif

Is this what you mean?

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 15, 2022 •
edited
Loading

Yes, that would work. Variations of that pattern are already in use elsewhere in the code base.

@ilg-ul ilg-ul mentioned this issue Apr 15, 2022
Closed
@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 15, 2022

I added a small test to verify that the definitions are generated on Windows (the test passed), and I also added some explicit mentions in the documentation.

@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 15, 2022

The CI checks passed.

Do I have to do anything more, or simply wait for a review?

@ilg-ul ilg-ul mentioned this issue Apr 16, 2022
Merged
@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 16, 2022

I created a new PR, hopefully cleaner: #42757.

@dead-claudia
Copy link
Author

@ilg-ul Could you fix the description of #42757 such that it doesn't close this issue? It doesn't actually fix this as this extends well beyond those two constants.

@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 18, 2022

I replaced Fixes with Refs. Is this ok?

Btw, the patch also changes the documentation to mention which constants are available on Windows. Isn't this what you asked for?

@dead-claudia
Copy link
Author

@ilg-ul

Btw, the patch also changes the documentation to mention which constants are available on Windows. Isn't this what you asked for?

I didn't realize that - didn't see it in the summary and I didn't actually look at the patch in question. In that case, this can be resolved by that.

@ilg-ul
Copy link
Contributor

ilg-ul commented Apr 18, 2022

You can also suggest a separate PR with explicit columns in that tables to show which constants are available for which platforms.

xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
@ilg-ul @xtx1130
src: define fs.constants.S_IWUSR & S_IRUSR for Win
9643691 
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: nodejs#41591

PR-URL: nodejs#42757
Refs: nodejs#41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Apr 28, 2022
@ilg-ul @targos
src: define fs.constants.S_IWUSR & S_IRUSR for Win
1976284 
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

PR-URL: #42757
Refs: #41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
juanarbol pushed a commit that referenced this issue May 31, 2022
@ilg-ul @juanarbol
src: define fs.constants.S_IWUSR & S_IRUSR for Win
1dcbf73 
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

PR-URL: #42757
Refs: #41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Jun 27, 2022
@ilg-ul @danielleadams
src: define fs.constants.S_IWUSR & S_IRUSR for Win
6d12faf 
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

PR-URL: #42757
Refs: #41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jul 12, 2022
@ilg-ul @targos
src: define fs.constants.S_IWUSR & S_IRUSR for Win
10eb4a8 
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

PR-URL: #42757
Refs: #41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Jul 31, 2022
@ilg-ul @targos
src: define fs.constants.S_IWUSR & S_IRUSR for Win
1281a48 
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

PR-URL: #42757
Refs: #41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
@ilg-ul @guangwong
src: define fs.constants.S_IWUSR & S_IRUSR for Win
2f8a2de 
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: nodejs/node#41591

PR-URL: nodejs/node#42757
Refs: nodejs/node#41591
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: define S_IWUSR & S_IRUSR for Windows xpack/node-fork
4 participants
@bnoordhuis @ilg-ul @dead-claudia @Mesteery