Skip to content

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
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
Labels
docIssues and PRs related to Node.js documentation.fsIssues and PRs related to file-system APIs and the fs module.good first issueIssues that are suitable for first-time contributors.

Description

@dead-claudia
dead-claudia
opened on Jan 19, 2022

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?

Activity

  1. dead-claudia
    added
    docIssues and PRs related to Node.js documentation.
    on Jan 19, 2022
  2. Mesteery
    added
    fsIssues and PRs related to file-system APIs and the fs module.
    on Jan 19, 2022

  3. ilg-ul commented on Apr 14, 2022

    @ilg-ul
    ilg-ul
    on Apr 14, 2022
    Contributor

    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.

  4. bnoordhuis
    added
    good first issueIssues that are suitable for first-time contributors.
    on Apr 15, 2022

  5. bnoordhuis commented on Apr 15, 2022

    @bnoordhuis
    bnoordhuis
    on Apr 15, 2022
    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.

  6. ilg-ul commented on Apr 15, 2022

    @ilg-ul
    ilg-ul
    on Apr 15, 2022
    Contributor

    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?

  7. bnoordhuis commented on Apr 15, 2022

    @bnoordhuis
    bnoordhuis
    on Apr 15, 2022
    Member

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

  8. ilg-ul commented on Apr 15, 2022

    @ilg-ul
    ilg-ul
    on Apr 15, 2022
    Contributor

    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.

  9. ilg-ul commented on Apr 15, 2022

    @ilg-ul
    ilg-ul
    on Apr 15, 2022
    Contributor

    The CI checks passed.

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

  10. ilg-ul commented on Apr 16, 2022

    @ilg-ul
    ilg-ul
    on Apr 16, 2022
    Contributor

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

  11. dead-claudia commented on Apr 18, 2022

    @dead-claudia
    dead-claudia
    on Apr 18, 2022
    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.

  12. ilg-ul commented on Apr 18, 2022

    @ilg-ul
    ilg-ul
    on Apr 18, 2022
    Contributor

    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?

  13. dead-claudia commented on Apr 18, 2022

    @dead-claudia
    dead-claudia
    on Apr 18, 2022
    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.

  14. ilg-ul commented on Apr 18, 2022

    @ilg-ul
    ilg-ul
    on Apr 18, 2022
    Contributor

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

  15. nodejs-github-bot
    closed this as completedin c6547c5on Apr 21, 2022
  16. xtx1130
    added a commit that references this issue on Apr 25, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    9643691
  17. targos
    added a commit that references this issue on Apr 28, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    1976284
  18. juanarbol
    added a commit that references this issue on May 31, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    1dcbf73
  19. danielleadams
    added a commit that references this issue on Jun 27, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    6d12faf
  20. targos
    added a commit that references this issue on Jul 12, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    10eb4a8
  21. targos
    added a commit that references this issue on Jul 31, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    1281a48
  22. guangwong
    added a commit that references this issue on Oct 10, 2022

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

    2f8a2de
  23. isheludko
    added a commit that references this issue on Mar 21, 2024

    src: define fs.constants.S_IWUSR & S_IRUSR for Win

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    docIssues and PRs related to Node.js documentation.fsIssues and PRs related to file-system APIs and the fs module.good first issueIssues that are suitable for first-time contributors.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions