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

doc: minor improvements in BUILDING.md #11963

Closed
wants to merge 1 commit into from
Closed

doc: minor improvements in BUILDING.md #11963

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Mar 21, 2017 •
edited
Loading

  1. necessarily reliably => necessarily reliable
  2. projects root directory => project's root directory
  3. remove console highlighting, as test alone is highlighted
  4. fix broken link for Android NDK
  5. highlight the directory location /usr/local/ssl/fips-2.0
  6. remove expected output for process.versions.openssl as the version
    displayed is not mentioned in the document
Checklist
Affected core subsystem(s)

doc

cc @mhdawson

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 21, 2017
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Mar 21, 2017
mhdawson
mhdawson approved these changes Mar 21, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

gibfahn
gibfahn reviewed Mar 22, 2017
View reviewed changes
BUILDING.md Outdated
8. Build Node.js with `make -j`
9. Verify with `node -p "process.versions.openssl"` (`1.0.2a-fips`)
9. Verify with `node -p "process.versions.openssl"`
Copy link
Member

@gibfahn gibfahn Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Verify with node -p "process.versions.openssl"

Maybe specify how you know the build was done in FIPS mode? Maybe:

  1. Verify that the openssl version contains "fips": node -p "process.versions.openssl" (for example1.0.2a-fips)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Trott
Trott reviewed Mar 22, 2017
View reviewed changes
BUILDING.md Outdated
@@ -26,7 +26,7 @@ Support is divided into three tiers:
the broader community.
* **Tier 2**: Full test coverage but more limited maintenance,
often provided by the vendor of the platform.
* **Experimental**: Known to compile but not necessarily reliably or with
* **Experimental**: Known to compile but not necessarily reliable or with
Copy link
Member

@Trott Trott Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think reliably is the correct word here. A fuller sentence would be: This is known to compile but it does not necessarily compile reliably or with a full passing test suite.

That said, all the qualifiers (known to, not necessarily, reliably...) are redundant. Maybe replace the whole sentence with something like this?:

May not compile reliably. Test suite may not pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to May not compile reliably or test suite may not pass.. Is that okay?

Trott
Trott reviewed Mar 22, 2017
View reviewed changes
BUILDING.md
@@ -121,7 +121,7 @@ and not a newer version.

To run the tests:

```console
Copy link
Member

@Trott Trott Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure I'm the one that added console. The idea was to distinguish between something that was a command (such as make test) and something which was console display (as it is here because it includes the $ prompt).

All that said, I don't actually care if it goes away. :-D

[UPDATE: Nope, I'm wrong about it being me; git blame says it was @ChALkeR.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm, I am leaving this as it is for now.

@thefourtheye thefourtheye force-pushed the fix-typo branch 2 times, most recently from 4df867a to 0747c53 Compare April 1, 2017 08:08
gibfahn
gibfahn suggested changes Apr 1, 2017
View reviewed changes
BUILDING.md Outdated
@@ -206,8 +205,7 @@ in the current continuous integration environment. The participation of people
dedicated and determined to improve Android building, testing, and support is
encouraged.

Be sure you have downloaded and extracted [Android NDK]
(https://developer.android.com/tools/sdk/ndk/index.html)
Be sure you have downloaded and extracted [Android NDK](https://developer.android.com/tools/sdk/ndk/index.html)
Copy link
Member

Choose a reason for hiding this comment

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

Could you do it like:

Download and extract the
[Android NDK](https://developer.android.com/tools/sdk/ndk/index.html),
then run:

? That way you keep it <80 chars and the link should still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@thefourtheye
doc: minor improvements in BUILDING.md
0089f85 
1. necessarily reliably => necessarily reliable
2. projects root directory => project's root directory
3. remove `console` highlighting, as `test` alone is highlighted
4. fix broken link for Android NDK
5. highlight the directory location `/usr/local/ssl/fips-2.0`
6. remove expected output for `process.versions.openssl` as the version
   displayed is not mentioned in the document
@thefourtheye
Copy link
Contributor Author

Landed in ba0e3ac

@thefourtheye thefourtheye deleted the fix-typo branch April 4, 2017 07:39
thefourtheye added a commit that referenced this pull request Apr 4, 2017
@thefourtheye
doc: minor improvements in BUILDING.md
ba0e3ac 
1. necessarily reliably => necessarily reliable
2. projects root directory => project's root directory
3. remove `console` highlighting, as `test` alone is highlighted
4. fix broken link for Android NDK
5. highlight the directory location `/usr/local/ssl/fips-2.0`
6. update expected output to an example for `process.versions.openssl` as the
   version displayed is not mentioned in the document

PR-URL: #11963

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 17, 2017
@thefourtheye @gibfahn
doc: minor improvements in BUILDING.md
6f86285 
1. necessarily reliably => necessarily reliable
2. projects root directory => project's root directory
3. remove `console` highlighting, as `test` alone is highlighted
4. fix broken link for Android NDK
5. highlight the directory location `/usr/local/ssl/fips-2.0`
6. update expected output to an example for `process.versions.openssl` as the
   version displayed is not mentioned in the document

PR-URL: #11963

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@thefourtheye @gibfahn
doc: minor improvements in BUILDING.md
b986105 
1. necessarily reliably => necessarily reliable
2. projects root directory => project's root directory
3. remove `console` highlighting, as `test` alone is highlighted
4. fix broken link for Android NDK
5. highlight the directory location `/usr/local/ssl/fips-2.0`
6. update expected output to an example for `process.versions.openssl` as the
   version displayed is not mentioned in the document

PR-URL: #11963

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@thefourtheye @MylesBorins
doc: minor improvements in BUILDING.md
b2e58b6 
1. necessarily reliably => necessarily reliable
2. projects root directory => project's root directory
3. remove `console` highlighting, as `test` alone is highlighted
4. fix broken link for Android NDK
5. highlight the directory location `/usr/local/ssl/fips-2.0`
6. update expected output to an example for `process.versions.openssl` as the
   version displayed is not mentioned in the document

PR-URL: #11963

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@thefourtheye @jasnell @Trott @cjihrig @mhdawson @gibfahn @mscdex @italoacasas @nodejs-github-bot