Ran into some issues getting started

[joneshf]

Heya! Just found out about spr, and I'm very intrigued! I've never used arc so I have no experience there. I've been interested in it for a while, but too lazy to set up Phabricator to actually try it out. So spr being arc inspired is great! It's way easier to get started with and see if this workflow makes sense. I'm really excited to see where spr ends up.

Gave it a try and ran into some issues. I'm going to dump them all in this issue just for the sake of not opening a bunch of different issues as my first interaction. Let me know if you'd prefer a different place to track these issues or separate issues. In any case, these are the things I ran into:

• [x] There are lots of requirements to build
  • Needs libiconv, libgit2, openssl, Apple's Security framework (probably comes from xcode tools or something).
  • Doesn't seem to matter if it's with cargo install spr or building from source.
• [x] Prebuilt macOS binary requires openssl.
  • Again, might come from xcode tools or something.
• [x] spr diff requires a Test Plan: section
  • This isn't really explained.
  • Have to guess one what the format is.
  • It'd be nice to specify what is actually required and allow different sections.
  • Some projects don't have a "Test Plan" section, but might have a "QA" section or a "Testing steps" or whatever else. Some might have multiple sections that are required.
  • Might be nice to somehow interact with PR templates (since this is a GitHub-specific tool).
• [x] spr land requires an approved review.
  • Not all repos require this.
  • Doesn't work for a single person working in a repo.
  • Admirable goal, but reality is that it needs to be a bit more flexible.
• [ ] https://github.com/getcord/spr/issues/117
  • Uses the --no-write-fetch-head option which was only introduced in 2.29.0.
• [x] spr diff requires additional scopes to update PRs.
  • It says it only needs repo and user scopes in the README.
  • If you only have the repo and user scopes, it'll error with:

      🛑  <query>:30:15: Your token has not been granted the required scopes to execute this query. The 'slug' field
    requires one of the following scopes: ['read:org', 'read:discussion'], but your token has only been granted
    the: ['repo', 'user'] scopes. Please modify your token's scopes at: https://github.com/settings/tokens.

• [x] spr diff says "Message (leave empty to abort):" and that doesn't work.
• [ ] #64
  • In fact, it will remove the GPG signature when it does whatever changes it does to update the commit.
• [x] Sections don't work as git trailers.
  • Might be intentional that they're not supposed to be trailers, but they basically look/feel like trailers.
  • Maybe this is some ad-hoc format that spr is introducing? If so, it would be nice to call out that they're explicitly not git trailers.
  • First reason is because they have spaces in them.
  • Second reason is because they aren't all at the end.
• [x] #63
  • Not entirely sure what this maps to in arc, maybe arc close-revision?

[sven-of-cord]

Hi @joneshf! Thanks a ton for giving spr a spin and all your feedback!

Let me go through the list of issues, you found:

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?
• the dependency on openssl is introduced by the reqwest library, in order to be able to make encrypted connections to the GitHub API. Having said that, we can configure the build to use rustls instead.
• two to-dos here: first improve documentation to mention and explain the "Test plan" section. The second would be to allow users to configure what sections they want in the commit message, and be able to make them optional or required. As of now, this configuration is hard-coded and insists on a test plan. (This is how the two Phabricator instances that I happened to have worked with were set up.) I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.
• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.
• we could query what version of git is installed, and not include the --no-write-fetch-head if it's not available. That means spr might overwrite your FETCH_HEAD file if you're on an older version of git, but that's probably better than not working at all.
• will fix the spr init help to include the right scopes!
• will fix abort-by-empty-message!
• I have to research what to do about GPG. We don't use it. GitHub signs the squash-merge commits, so in our git history each commit is signed by GitHub, but we haven't bothered with GPG for signing commits that you submit for code review.
• Sections are not git trailers, and I don't think they should be. They are not meant to be metadata key-value pairs, but just part of structured commit messages (i.e. part of the commit message payload).
• closing PRs is also a good idea. I have just done that via the GitHub ui.

Wow, that's a lot of stuff. Thanks again for all this feedback. I can quickly fix a few of them, but will open issues for the remaining ones (citing your issue).

[joneshf]

Thanks for taking the time to respond to all this stuff. I kind of just dumped a wall of text on you there.

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for 😅) is only to document the dependencies rather than change them in some way.

• I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.

Now that you mention it, I'm not entirely sure it would be a useful experience if the authoring of commits is done outside of spr. I was kind of thinking that spr would use the PR template to validate the commit message when doing spr diff, but that would mean parsing and working with markdown (which can be pretty hard). If you could configure the required sections somehow, that's probably good enough. Though, it might still need to be able to at least parse sections differently Test Plan: vs. ## Test Plan vs. some other thing.

• Sections are not git trailers, and I don't think they should be. They are not meant to be metadata key-value pairs, but just part of structured commit messages (i.e. part of the commit message payload).

Makes sense. I wasn't entirely sure, so I appreciate the explanation!

[joneshf]

• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.

I will say about this one that I kind of do appreciate the idea of spr doing its own validation on whether a PR can land or not. Maybe if there's configuration that happens for spr this could be one of the things that could be configured irrespective of what the settings are on GitHub.

[sven-of-cord]

Thanks for taking the time to respond to all this stuff. I kind of just dumped a wall of text on you there.

You're most welcome. Thanks again for raising these issues.

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for sweat_smile) is only to document the dependencies rather than change them in some way.

Ah, now I get it. I assume you installed a binary from https://github.com/getcord/spr/releases - I kind of forgot we had those. I set up those build before I did the homebrew formula. I am tempted to stop making them and refer Mac users to homebrew - because that obviously handles the dependency management. Or do you personally prefer installing a gzipped binary? (I am not a Mac user myself, so I try to gauge if there is use for having the binary releases here in addition to homebrew).

On that note, I did go through some dependencies and deselected some unused features, as well as switched to rustls. Future binaries should not depend on openssl anymore.

• I am not sure how spr should interact with pull request templates, as writing the messages is not done in spr. We use git config commit.template and point it to a checked-in file (.gitmessage.template) to have a shared template for out git commit messages (which then become pull request descriptions). This could be a good solution, especially if spr was more configurable in its handling of commit messages.

Now that you mention it, I'm not entirely sure it would be a useful experience if the authoring of commits is done outside of spr. I was kind of thinking that spr would use the PR template to validate the commit message when doing spr diff, but that would mean parsing and working with markdown (which can be pretty hard). If you could configure the required sections somehow, that's probably good enough. Though, it might still need to be able to at least parse sections differently Test Plan: vs. ## Test Plan vs. some other thing.

That sounds like a good idea. Will have to think about the details. Maybe spr could be configured to produced either plain-text-style or markdown-style commit message. The parser could of course understand both "Test plan:" and "## Test plan", but spr would consistently produce the preferred flavour.

• good point about spr land and requiring reviews. I think we can just skip this check, actually. If you configure GitHub to require accept to merge, then it should refuse spr's merge API command if the PR wasn't accepted.

I will say about this one that I kind of do appreciate the idea of spr doing its own validation on whether a PR can land or not. Maybe if there's configuration that happens for spr this could be one of the things that could be configured irrespective of what the settings are on GitHub.

Why not. The next release will have a evaluate the spr.requireApproval config, which will default to false.

I am also working on adding more/better documentation. Plus, a bug caused by flaky GitHub behaviour that occasionally left the pull request un-updateable by spr is getting fixed.

So stay tuned! :wink:

[joneshf]

• the list of requirements looks about right: libgit2 is fundamental to what spr does. libiconv would be needed for unicode handling. openssl is probably needed to make GitHub API requests, to connect via https. Are any of those problematic in particular, or are you just generally interested in keeping the number of dependencies low?

Sorry, I should've explained a bit more. I'm not particularly concerned about which dependencies are needed nor how many of them there are. Whether you trim down the dependencies is up to you. It might make it easier to get installed (maybe also maintain), but they all seem fairly standard for what's going on here. It's more that I had to figure it out by trial and error. The ask (that I forgot to actually ask for sweat_smile) is only to document the dependencies rather than change them in some way.

Ah, now I get it. I assume you installed a binary from https://github.com/getcord/spr/releases - I kind of forgot we had those. I set up those build before I did the homebrew formula. I am tempted to stop making them and refer Mac users to homebrew - because that obviously handles the dependency management. Or do you personally prefer installing a gzipped binary? (I am not a Mac user myself, so I try to gauge if there is use for having the binary releases here in addition to homebrew).

On that note, I did go through some dependencies and deselected some unused features, as well as switched to rustls. Future binaries should not depend on openssl anymore.

I think I'm a bit of an outlier. I don't actually have brew installed on this macOS machine. I tend to use nix to manage my dependencies. So, I dunno if I'm the one you want to respond to your questions. I imagine most people on macOS would use brew.

[sven-of-cord]

@joneshf - I am trying to get spr into nixpkgs, see https://github.com/NixOS/nixpkgs/pull/178168

[joneshf]

Thanks for adding that!

[joneshf]

Looks like everything except the git version one has been addressed or has an issue tracking it. I'm going to turn that last one into an issue and close this since it will be tracked there.