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

doc: add statement about support for dependencies #11942

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

  • In the review of doc: add supported platforms list #11872 we
    pulled out the discussion of supporting dependencies. This
    PR adds back that statement.
  • Update README.md to indicatte BUILDING.md contains the list
    of supported platforms. ( I missed this part in the original PR)
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

doc

- In the review of nodejs#11872 we
pulled out the discussion of supporting dependencies.  This
PR adds back that statement.
- Update README.md to indicatte BUILDING.md contains the list
of supported platforms.
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 20, 2017
@mhdawson mhdawson mentioned this pull request Mar 20, 2017
3 tasks
@mhdawson
Copy link
Member Author

To start the conversation where it was left of in the last PR @bnoordhuis suggested we not include a statement about dependency support:

I'd leave this out. With c-ares it was broken for years. The numerous V8 patches we float basically mean it's only guaranteed to work with our fork.

Historically, we made no promises except that we'll consider patches.

### Shared libraries & dependencies

Node.js intends to support building against shared representations of
dependencies found in the [*deps*](./deps/) directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no deps directory in the doc/api directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, prefer [..][..] format to mention links.

@mhdawson
Copy link
Member Author

@thefourtheye, BUILDING.md is not in in the doc/api directory. Its at the top level with the README.md so the link seems to work ok.

In terms of [..][..], it was originally in that format but it was suggested in the review of the last PR that it be changed to .. as it seems people were not aware the [..][..] format was going to work.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Mar 20, 2017
Copy link
Member

@lpinca lpinca 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 @thefourtheye's nit addressed.

@bnoordhuis
Copy link
Member

I'm ambivalent. This has potentially far-ranging implications and "intends to support" is stretching it when we don't do any actual testing. "If it works, great; if not, send pull requests" would be more honest.

@mhdawson
Copy link
Member Author

@rvagg can you chime in here ?

Since we don't test building with shared libraries I'd favor just leaving it out.

@mhdawson
Copy link
Member Author

mhdawson commented May 9, 2017

ping @rvagg. Would like your input to decide if we should add a statement with respect to dependencies or just close this PR.

@mhdawson mhdawson closed this Jun 28, 2017
@mhdawson mhdawson deleted the docsupport2 branch June 28, 2017 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants