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

Vote on primordials in error path #1158

Merged
merged 25 commits into from
Feb 23, 2022
Merged

Vote on primordials in error path #1158

merged 25 commits into from
Feb 23, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 26, 2022

I've found the subject of voting in TSC comes quite often, and we often have recurrent discussions on what tool to use. I've decided to take upon me the creation of a tool to help us vote using CLI and GitHub UI. It might end up being a terrible idea, or a genius one, but at least we would have try. If it's successful, we can talk about officially adopt this tool for Node.js related votes.

The vote options were presented in #1104 (comment).

Use one of the following methods to case a vote (or abstain):

Vote using npx+gh (requires Node.js 16+, and git)

Requires gh (GitHub CLI tool) to be installed, and logged in to your GitHub account.

npx --package=@aduh95/caritat voteOnGitHub https://github.com/nodejs/TSC/pull/1158

If you want to abstain, you can use the --abstain flag (it will create a ballot with all the scores set to 0):

npx --package=@aduh95/caritat voteOnGitHub https://github.com/nodejs/TSC/pull/1158 --abstain
Vote using npx (requires Node.js 16+, and git)
# Cloning and pushing using SSH
npx --package=@aduh95/caritat voteUsingGit -r git@github.com:nodejs/TSC.git \
  -b vote-primordials-error-path -p votes/primordials-error-path

# Cloning and pushing using HTTPS
USERNAME=<your-github-handle> \
npx --package=@aduh95/caritat voteUsingGit -r https://$USERNAME@github.com/nodejs/TSC.git \
  -b vote-primordials-error-path -p votes/primordials-error-path --handle "$USERNAME"

# To abstain, use the flag --abstain
npx --package=@aduh95/caritat voteUsingGit -r git@github.com:nodejs/TSC.git \
  -b vote-primordials-error-path -p votes/primordials-error-path --abstain
Vote using sh (requires git, and OpenSSL CLI)

Caution: LibreSSL CLI is not compatible, please use OpenSSL 1.1.1+.

curl -LOJ https://github.com/stduhpf/caritat/raw/HEAD/sh/voteUsingGit.sh
curl -LOJ https://github.com/stduhpf/caritat/raw/HEAD/sh/encryptBallot.sh
chmod +x encryptBallot.sh
sh ./voteUsingGit.sh \
  <your-github-handle> \
  git@github.com:nodejs/TSC.git \
  vote-primordials-error-path \
  votes/primordials-error-path
rm encryptBallot.sh
rm voteUsingGit.sh
Vote using PowerShell (requires git, and OpenSSL CLI)

Caution: LibreSSL CLI is not compatible, please use OpenSSL 1.1.1+.

Set-ExecutionPolicy Bypass -Scope Process -Force
[System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072
$tmpFile = $(New-TemporaryFile | Rename-Item -NewName { $_ -replace 'tmp$', 'ps1'} -PassThru)
$encryptBallot = $tmpFile -replace '\\[^\\]+$','\encryptBallot.ps1'
((New-Object System.Net.WebClient).DownloadString('https://raw.githubusercontent.com/stduhpf/caritat/HEAD/sh/voteUsingGit.ps1')) | Out-File -FilePath $tmpFile
((New-Object System.Net.WebClient).DownloadString('https://raw.githubusercontent.com/stduhpf/caritat/HEAD/sh/encryptBallot.ps1')) | Out-File -FilePath $encryptBallot
& $tmpFile `
  <your-github-handle> `
  git@github.com:nodejs/TSC.git `
  vote-primordials-error-path `
  votes/primordials-error-path
Remove-Item $encryptBallot
Remove-Item $tmpFile
Vote using the web
  1. Fill in the ballot at https://stduhpf.github.io/caritat/#https://github.com/nodejs/TSC/pull/1158. It will copy the encrypted ballot to your clipboard.
  2. Paste the encrypted ballot in https://github.com/nodejs/TSC/new/vote-primordials-error-path/votes/primordials-error-path. It's recommended to use <your-github-handle>.json for the file name.
  3. Commit directly to the vote-primordials-error-path branch.

To abstain, set all scores to 0 before encrypting the ballot.

This will encrypt a file containing your preferences, file that will be decrypted when doing the counting in two weeks or so.

The counting will be done using the Condorcet method.

Refs: #1104 (comment)

Bugs should be sent to: https://github.com/stduhpf/caritat

@Trott
Copy link
Member

Trott commented Jan 26, 2022

@aduh95 Is there a public source code repository for the tool? It seems to be https://github.com/stduhpf/caritat but with some additional files. Maybe there's a tag/branch/fork I should be looking at? I did the tsc compilation step, but I still don't have (for example) app.js.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 26, 2022

@aduh95 Is there a public source code repository for the tool? It seems to be https://github.com/stduhpf/caritat but with some additional files. Maybe there's a tag/branch/fork I should be looking at? I did the tsc compilation step, but I still don't have (for example) app.js.

Yes, it is https://github.com/stduhpf/caritat. app.js has been deleted, it was a left-over from a not-so-clean local repo. I've published v0.2.0 that fixes that, repo and npm package should be in sync now.

@Trott
Copy link
Member

Trott commented Jan 26, 2022

Yes, it is https://github.com/stduhpf/caritat. app.js has been deleted, it was a left-over from a not-so-clean local repo. I've published v0.2.0 that fixes that, repo and npm package should be in sync now.

Awesome, thanks.

@Ginden

This comment was marked as off-topic.

@MylesBorins
Copy link
Contributor

this is very very cool. Will dig more into implementation in a bit

@aduh95

This comment was marked as off-topic.

@mhdawson
Copy link
Member

mhdawson commented Feb 2, 2022

@aduh95 its not immediately obvious if we put higher scores for options we want my guess is that if I wanted to rank them from highest preference to lowest I could use 0-5 for the scores where 5 is the one I prefer the most? Can you confirm that is correct?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 2, 2022

@aduh95 its not immediately obvious if we put higher scores for options we want my guess is that if I wanted to rank them from highest preference to lowest I could use 0-5 for the scores where 5 is the one I prefer the most? Can you confirm that is correct?

@mhdawson that would works, absolutely. As long you put integers, you can use whichever scale that's more convenient for you (negative numbers also works btw): the largest score for the option(s) you prefer the most (5 in your example), the lowest score for the option(s) you prefer the less (0 in your example).

@mhdawson
Copy link
Member

mhdawson commented Feb 2, 2022

@aduh95 thanks

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 3, 2022

@nodejs/tsc Reminder that there's an ongoing vote on this PR, you are invited to participate; see the OP for instructions, don't hesitate to ask questions if you have any.
Vote is closing next week if we have quorum.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 3, 2022

@jasnell you mentioned on the issue you don't have a preference but there's one option you don't like. FWIW you can cast a vote that set all the options to 0 (i.e. abstain) except for the issue you don't like that you can set to -1, that will be taken into account when counting the ballots.

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 5, 2022

Running into an EACCES when I'm trying to vote using sh - it commits an empty file for me. Reported to nodejs/caritat#3.

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 5, 2022

I used the web ui to generate the encrypted ballot this time and after pushing I realize that it doesn't include the author info (reported in nodejs/caritat#5). Adding it manually.

@tniessen
Copy link
Member

Use primordials in the error path only if there are no significant perf regression.

I am confused by the winning option. Does it explicitly support primordials? In other words, could this option be rephrased as Use primordials in the error path unless there are significant perf regressions but not as Do not use primordials in the error path if there are significant perf regressions?

@RaisinTen
Copy link
Contributor

To my understanding, it means - if someone volunteers to add primordials to error paths, we are likely to accept it if and only if there are no significant perf regressions.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 16, 2022

And the reciprocal would be – if someone opens a PR to remove primordials from error paths, it would be rejected, unless said primordials was causing a measurable perf regression.
If we do other votes related to primordials in the future, I agree we should try to make that wording less confusing.

@tniessen
Copy link
Member

we are likely to accept it if and only if there are no significant perf regressions.

Does that mean writing benchmarks for error paths?

@Trott
Copy link
Member

Trott commented Feb 16, 2022

Given that this vote has been completed, should this be moved out of draft mode and landed?

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 16, 2022

Given that this vote has been completed, should this be moved out of draft mode and landed?

The next step would be for me (or anyone that wants to beat me to it) to decrypt all the ballots, then I think it can be merged. Maybe we should also create a .json file similar to 2021-09-29-0.json so participation in this vote can be counted by the automation.

@RaisinTen
Copy link
Contributor

Does that mean writing benchmarks for error paths?

I don't think there is any other way to make sure that the performance is not degraded. So IIUC, PRs like nodejs/node#41702 that attempt to add primordials to an error path would be blocked because we still don't have benchmarks testing the error path.

@ljharb
Copy link
Member

ljharb commented Feb 16, 2022

I’m confused; what about code not in the error path?

@tniessen
Copy link
Member

Does that mean writing benchmarks for error paths?

I don't think there is any other way to make sure that the performance is not degraded.

That's... interesting.

I’m confused; what about code not in the error path?

That was out of scope for this vote... I believe the intention is for more votes to follow.

@targos
Copy link
Member

targos commented Feb 16, 2022

Does that mean writing benchmarks for error paths?

I don't think there is any other way to make sure that the performance is not degraded.

That's... interesting.

Interesting indeed. I thought that we were voting on the error paths because performance regressions that only happen there are not problematic.
My understanding was that we would only block a change in an error path if that somehow affects the performance of non-error paths.

@Trott
Copy link
Member

Trott commented Feb 17, 2022

Concurring with @targos and @tniessen in that I don't think we've ever been terribly concerned about error-path performance. I could be mistaken, but it seems like an odd place to put in extra effort on performance.

votes/2022-02-21-0.json Outdated Show resolved Hide resolved
@aduh95 aduh95 marked this pull request as ready for review February 21, 2022 21:49
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

@mhdawson
Copy link
Member

mhdawson commented Feb 21, 2022

Concurring with @targos and @tniessen in that I don't think we've ever been terribly concerned about error-path performance. I could be mistaken, but it seems like an odd place to put in extra effort on performance.

For me I found it a bit difficult to be voting about the error path without having discussed if we should use for the main paths. As such I voted more conservatively than I might otherwise have.

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

I agree that we have not been concerned about the error cases performance a lot and as such it's probably fine. I just wonder why we added that option to our vote if we do not plan to actually handle it different from "Make the code in the error path full tamper proof by using primordials" as it seems like the overall vote would have been different in that case.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Tested this with the tooling and it worked great. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.