Skip to content

Conversation

@arminsabouri
Copy link
Collaborator

@arminsabouri arminsabouri commented Aug 27, 2025

We don't actually use apt in this script. And its possible apt is not the prefered package manager of the distro.

Pull Request Checklist

Please confirm the following before requesting review:

  • A human has reviewed every single line of this code before opening the PR (no auto-generated, unreviewed LLM/robot submissions).
  • I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

We don't actually use apt in this script. And its possible apt is not
the prefered package manager of the distro.
@arminsabouri
Copy link
Collaborator Author

Wooo PR number 1000!

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 17275565469

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 85.717%

Totals Coverage Status
Change from base Build 17270958703: 0.0%
Covered Lines: 7970
Relevant Lines: 9298

💛 - Coveralls

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

utACK 8153a94

@arminsabouri arminsabouri merged commit 811848d into payjoin:master Aug 27, 2025
14 checks passed
@arminsabouri arminsabouri deleted the remove-apt-cmds-from-ffi-gen branch August 27, 2025 18:55
@nothingmuch
Copy link
Collaborator

We came across this in #997, which should simplify the script... It's not just weird to run sudo it's also that apt is not portable.

@benalleng
Copy link
Collaborator

Ahh makes sense that this shouldn't have been in this script specifically with os variations, I pulled it out from the workflow file here 363d2b5#diff-092659a9bd0068791326fb1d08f005532e5aeb983b999a7bd32c51deee0a71d5L42-L43 should we put it back there since we are specifically working with an ubuntu-latest image?

benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Aug 27, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
benalleng added a commit to benalleng/rust-payjoin that referenced this pull request Aug 27, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
DanGould pushed a commit to DanGould/rust-payjoin that referenced this pull request Aug 28, 2025
I moved this code incorrectly in payjoin#989 to a general binding build script but
others not using a debian based system immediately ran into problems running
this apt based package manager on their machine.

It was removed in payjoin#1000 from the binding script but This probably should
just be returned to the workflow it was originally.
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.

5 participants