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: update min mac ver + move mac arm64 to tier 1 #39586

Closed
wants to merge 1 commit into from

Conversation

AshCripps
Copy link
Member

Update the minimum macos version that can compile to match the
xcode requirements.

Also move mac arm64 to tier 1.

refs: #39584 (comment)

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Jul 30, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jul 30, 2021

/cc @nodejs/build

richardlau
richardlau previously approved these changes Jul 30, 2021
BUILDING.md Outdated
@@ -113,8 +113,8 @@ platforms. This is true regardless of entries in the table below.
| Windows | x86 (native) | >= Windows 8.1/2012 R2 | Tier 1 (running) / Experimental (compiling) <sup>[6](#fn6)</sup> | |
| Windows | x64, x86 | Windows Server 2012 (not R2) | Experimental | |
| Windows | arm64 | >= Windows 10 | Tier 2 (compiling) / Experimental (running) | |
| macOS | x64 | >= 10.13 | Tier 1 | |
| macOS | arm64 | >= 11 | Experimental | |
| macOS | x64 | >= 10.14.4 | Tier 1 | |
Copy link
Member

Choose a reason for hiding this comment

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

Actually do the binaries run on macOS 10.13? This table is the table of platforms we support running Node.js on but not a guarantee that you can build on this (building requirements are under the "Supported toolchains" section below).

The reason for asking is that usually we wouldn't change this table during a release (e.g. 16.x) and would normally consider raising versions to be semver-major.

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea as we dont test on 10.13 - the minimum version is set to target 10.13 but I dont think its ever been verified.

Copy link
Member

Choose a reason for hiding this comment

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

This number really should match MACOSX_DEPLOYMENT_TARGET in common.gypi, and also see lines 175 and 176 below in this file.

MACOSX_DEPLOYMENT_TARGET has been a pretty solid tool historically for Node.js, it compiles the binaries to be compatible with the version we set there, even though we haven't always (maybe never) tested that far back. I think we should either match that value here (update it, or update this), or have some kind of note which makes it clear that we compile binaries to be supported on that version.

If you feel strongly enough that this column should be out of sync with MACOSX_DEPLOYMENT_TARGET then I'd suggest using the Notes column to describe what we do and that binaries in theory should be compatible back to that version but we recommend running it with the version you're putting here.

Copy link
Member Author

@AshCripps AshCripps Jul 30, 2021

Choose a reason for hiding this comment

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

I prefer that approach - Ill change this back to 10.13 and add an note making it explicit that whilst we supporting our own binaries running on that level we dont necessarily support compiling at that level.

BUILDING.md Outdated Show resolved Hide resolved
Copy link
Member

@richardlau richardlau 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 lint warnings fixed.

BUILDING.md Outdated
Comment on lines 159 to 160
However there is no guarantee compiling on 10.13 will work as Xcode11 is required
to compile.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
However there is no guarantee compiling on 10.13 will work as Xcode11 is required
to compile.
However there is no guarantee compiling on 10.13 will work as Xcode11 is
required to compile.

Lint.

Update the minimum macos version that can compile to match the
xcode requirements.

Also move mac arm64 to tier 1.

refs: nodejs#39584 (comment)
@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 1, 2021
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2021

Landed in 4f9fd8d...4d60ee8

@github-actions github-actions bot closed this Aug 1, 2021
nodejs-github-bot pushed a commit that referenced this pull request Aug 1, 2021
Update the minimum macos version that can compile to match the
xcode requirements.

Also move mac arm64 to tier 1.

refs: #39584 (comment)

PR-URL: #39586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@AshCripps AshCripps deleted the update-mac-doc branch August 2, 2021 10:16
danielleadams pushed a commit that referenced this pull request Aug 16, 2021
Update the minimum macos version that can compile to match the
xcode requirements.

Also move mac arm64 to tier 1.

refs: #39584 (comment)

PR-URL: #39586
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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.

10 participants