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: change smartos support type to experimental #56220

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Dec 11, 2024

Follow up for nodejs/TSC#1663

cc @nodejs/build @nodejs/tsc @nodejs/platform-smartos

@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 Dec 11, 2024
BUILDING.md Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/lower-smartos-experimental branch from fc1cf65 to 9e99d90 Compare December 11, 2024 01:04
@bahamat
Copy link

bahamat commented Dec 11, 2024

I've replied to nodejs/TSC#1663. Given my comments there, I think it's premature (at best) to demote SmartOS. I vote to close this PR without merging.

Copy link

@bahamat bahamat left a comment

Choose a reason for hiding this comment

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

Further discussion needed as per nodejs/TSC#1663.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 11, 2024
@marco-ippolito
Copy link
Member

Are there other cases of tier 2 platforms that we don't release binaries for?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Considering that is now working properly, I think it’s ok.

@anonrig
Copy link
Member Author

anonrig commented Dec 18, 2024

Considering that is now working properly, I think it’s ok.

I disagree.

@joyeecheung
Copy link
Member

I opened nodejs/TSC#1666 as an alternative - IMO, we should change how we treat tier 2 platforms (i.e. don't insist on keeping all tests green in every PR on them) to make it sustainable. Otherwise there's effectively no difference between tier 1 and tier 2, and basically only tier 1 and experimental, which would lead to this PR.

@anonrig
Copy link
Member Author

anonrig commented Jan 8, 2025

I agree with @joyeecheung's comment on this issue, and believe we should proceed with this pull-request: nodejs/TSC#1666 (comment)

Feel free to open a vote or a new issue if you want to continue discussing. I think people have been overestimating the marginal benefit it brings to test platforms depended by <1% of the user base in every single node-test-pull-request CI v.s. just in node-daily-master CI (or additional node-test-commit CI in deps upgrades/PRs considered as risky by the platform teams, if need be), or underestimating the cost of relying on volunteers to triage additional flakes on these platforms under the current miserable situation of the test suites, allowing the platform teams to be too reactive instead of being proactive about the share of CI instability these platforms contribute to, but I don't have enough energy for this debate.

For visibility: @nodejs/tsc

@mcollina
Copy link
Member

mcollina commented Jan 8, 2025

100% of the maintainers of @nodejs/platform-smartos have asked that PRs are runs against those machines.

@bahamat
Copy link

bahamat commented Jan 8, 2025

100% of the maintainers of @nodejs/platform-smartos have asked that PRs are runs against those machines.

Yes, that's correct. I believe this was also something that the RH/IBM/AIX folks wanted, so SmartOS/illumos is not alone here.

The new SmartOS build servers have been running smoothly since mid-December. During that time there was one issue raised to us that was resolved pretty much right away.

@jasnell
Copy link
Member

jasnell commented Jan 8, 2025

Running all PRs against those machines is fine. I think the question is whether failures on those machines should be considered blocking in order for the PR to land, especially if the reason those fail is due to the CI image or compiler environment being out of date or, at least, not up to the minimum requirements.

Right now, if the smartos CI runners are updated and working correctly then I would say that the current issue is resolved, at least for now. But longer term we should address the larger question here given how disruptive these kinds of issues have been to landing PRs. We've had PRs blocked for months due to CI issues like this and I don't want to get back into that kind of state.

If there are still issues with the smartos runners blocking PRs right now, let's list those issues and get them dealt with now.

@bahamat
Copy link

bahamat commented Jan 9, 2025

If there are still issues with the smartos runners blocking PRs right now, let's list those issues and get them dealt with now.

I wholeheartedly agree with this statement. To my knowledge there are zero PRs blocked by SmartOS. If I am mistaken in this, I would like to know so that we can address it as soon as possible.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

For now, let's leave SmartOS at Tier 2.

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. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants