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

dogecoin: restore #341302

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

dogecoin: restore #341302

wants to merge 3 commits into from

Conversation

cleverca22
Copy link
Contributor

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@craigem
Copy link
Contributor

craigem commented Sep 12, 2024

I approve the addition of myself as a maintainer.

@RaghavSood
Copy link
Member

Please run the formatter as well.

@RaghavSood
Copy link
Member

Result of nixpkgs-review pr 341302 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • dogecoin
1 package built:
  • dogecoind

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Sep 12, 2024
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Sep 12, 2024
@JohnRTitor JohnRTitor requested review from drupol and wegank September 12, 2024 13:30
drupol
drupol previously requested changes Sep 12, 2024
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for the PR.

  1. Can you please adjust the PR title, it should be: dogecoin: init at <version>
  2. Can you fix the minor comments I added?
  3. Make sure the PR only contains 1 single commit
  4. Fix the commit log message and PR title according to the contributors guidelines

Cocoa ? null,
}:

stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use the finalAttrs pattern here.

The finalAttrs pattern will let you remove the rec keyword (see its implementation in Nix).

Why is this pattern preferred to rec ?

Let's take this simple code example:

mkDerivation rec {
   foo = 1;
   bar = foo + 1;
}

and then .overrideAttrs(old: { foo = 2; }), you'll get { foo = 2; bar = 2; } while with finalAttrs pattern, it would work correctly because it's a real fixed point.

Let me share a couple of useful links regarding the finalAttrs pattern:

  1. History: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293
  2. Documentation: https://nixos.org/manual/nixpkgs/unstable/#mkderivation-recursive-attributes
  3. Recent example of implementation: https://github.com/NixOS/nixpkgs/compare/17f96f7b978e61576cfe16136eb418f74fab9952..9e6ea843e473d34d4f379b8b0d8ef0425a06defe

Feel free to reach out if you need some assistance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didnt really write that rec part, and i do personally try to avoid it

if you look at each commit seperately, youll see the first one is a revert of somebody deleting dogecoin entirely, which is where the rec came from
and the next commit is just updating the version, and disabling upnp

Copy link
Contributor

@oxalica oxalica Sep 15, 2024

Choose a reason for hiding this comment

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

i didnt really write that rec part, and i do personally try to avoid it
if you look at each commit seperately, youll see the first one is a revert of somebody deleting dogecoin entirel

You could argue that rec is not worse than finalAttrs (which I might say). But your reason does not seem valid to me. "It is written by someone else" or "it used to be in nixpkgs" does not imply it meets the current practice. Legacy means legacy for a reason.

This should be reviewed as a new-package PR, because it adds a package which did not exist. You could reference the previous commit, but now that code is under your contribution and you are responsible for it.

But anyway, I think rec or finalAttrs are both acceptable for this PR.

owner = "dogecoin";
repo = "dogecoin";
rev = "v${version}";
sha256 = "sha256-Jmdxaona9bI9mw+WDGnrFU2ETgVTTQ7HHaWRodK/c4k=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sha256 = "sha256-Jmdxaona9bI9mw+WDGnrFU2ETgVTTQ7HHaWRodK/c4k=";
hash = "sha256-Jmdxaona9bI9mw+WDGnrFU2ETgVTTQ7HHaWRodK/c4k=";


enableParallelBuilding = true;

meta = with lib; {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing meta.mainProgram

@drupol
Copy link
Contributor

drupol commented Sep 12, 2024

No worries, I understand what's happening here :) But if we are to introduce things in nixpkgs, let's make it right from the outset, and this PR is actually a good opportunity to do it.

@Atemu
Copy link
Member

Atemu commented Sep 15, 2024

@drupol you are free to improve any packages you like but this is just a revert and even if this was a brand new package, the code would be acceptable.

I see no reason to block this PR for these unsubstantial nitpicks.

@Atemu Atemu dismissed drupol’s stale review September 15, 2024 11:07

Nitpicks are not blocking.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Could you reword the update commit to follow the standard format of update commits in Nixpkgs?

With that I think we're good to go here.

Comment on lines +95 to +100
maintainers = with maintainers; [
edwtjo
offline
cleverca22
craigem
];
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be best to remove the maintainers who did not explicitly agree, even if this is just a revert.

@Atemu Atemu changed the title Restore dogecoin dogecoin: restore Sep 15, 2024
@drupol
Copy link
Contributor

drupol commented Sep 15, 2024

My approach with large repositories like this one is to avoid making unnecessary commits whenever possible. Reducing the number of commits is beneficial for processes like git bisect, and fewer commits and PRs require less energy to maintain the project, host it, and consume fewer resources overall. I believe we should aim to make each commit as optimal as possible, especially when it's straightforward to do so.

The effort to improve the current PR is minimal, and even though it could be accepted as is, I don't see why the changes couldn't be made directly here. Your comment hasn't convinced me at all and I still don't consider my changes as nitpicks.

That said, I'm not planning to leave the project (see context: #341075) if this gets merged as is, but you can expect less involvement from me and, consequently, more work for other contributors. Thank you!

@Atemu
Copy link
Member

Atemu commented Sep 15, 2024

It's useful to reduce the amount of commits especially when using git bisect.

When talking about git bisect time you must keep in mind that bisect steps merely grow by log2(n) where n is the number of commits between two bisect points. If we had twice as many commits per time unit, that'd only cause one additional step in a bisect across the same time "distance". A bit of back of the napkin math tells me it'd increase the steps for a bisect between commits one year apart from 17 to 18 to give you an idea of the scale. Again, that's with an assumed double the number of commits/time that we currently have.

It's actually even less meaningful of a difference in the context of Nix because the case at hand is splitting the same amount of meaningful change into smaller units of change and it's only really the meaningful changes that incur any significant cost during a bisect because unsubstantial changes eval to the same result and are therefore cached.

And that's also ignoring useful tricks like first-parent bisect which makes git bisect scale with the number of PRs against master; decoupling it from the amount of commits per PR entirely.

As long as we're not talking about a quadratic increase in PR throughput, we don't need to worry about our ability to bisect any time soon.

IMO allowing humans to understand changes better is so much more important than keeping some number down that only affects automated tooling in a small way at worst.

The effort to improve the current PR is minimal, and even if it would be accepted as it is, I don't see why it couldn't be made here, directly.

Noting nits is fine IMHO (opinions differ on this) but they should not block a meaningful change for no good reason. I agree that it'd be nice to modernise the derivation a bit but that's simply not the goal of this PR. If @cleverca22 is fine with the derivation as it was, I see no reason to disallow it.

More commits and PRs requires more energy, more time, more work and more resources in general, and I do believe that we should reduce these as much as possible in making sure that what we commit is optimal.

What requires significant human resources are meaningful or significant changes. Tiny refactors such as the ones you suggested don't require much reviewer attention. If I saw a stand-alone PR only consisting of such changes and the CI was green, I'd hit the merge button and close the tab within seconds.

Such changes are simply not the kind that's driving our review shortage in any significant way.

Anyway, don't expect me to leave the project (context: #341075) if this gets merged as is, just expect less involvement from my side, ... And more work for other contributors.

I'm not sure what you're tying to tell me here. Are you implying that the ability to nitpick PRs to death is somehow critical for your involvement in Nixpkgs?

FWIW I don't desire anyone leaving the project (even people I have trouble with on a personal level sometimes) but that also applies to contributors who do not have merge privileges.
If the typical contributing experience to Nixpkgs is that a gatekeeper blocks your simple PR because they want to see minor adjustments to the code to better fit their personal preferences in packages they don't even maintain that are unrelated to the core purpose of the PR, that's bad. Extremely bad IMHO.
This is not directed at you in particular btw, I've been absolutely guilty of this too. It's super easy to fall into this behaviour without realising it. I see the only way to change this unfortunate status quo is to call out this behaviour whenever I notice it happening.

Copy link
Contributor

@oxalica oxalica left a comment

Choose a reason for hiding this comment

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

This PR actually added dogecoind, but dogecoin is still marked broken by broken = withGui. The build failure of dogecoin seems to be compilation requiring at least C++17.

By adding this line, it compiles and runs for me:
CXXFLAGS = lib.optionalString withGui "-std=c++17";

I don't know how to test the GUI. But if it works without issue, please add the flag above and remove the broken flag.


Result of nixpkgs-review pr 341302 run on x86_64-linux 1

1 package marked as broken and skipped:
  • dogecoin
1 package built:
  • dogecoind

qtbase ? null,
wrapQtAppsHook ? null,
qttools ? null,
qmake ? null,
Copy link
Contributor

Choose a reason for hiding this comment

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

qmake is unused.

@drupol
Copy link
Contributor

drupol commented Sep 15, 2024

When talking about git bisect time you must keep in mind that bisect steps merely grow by log2(n) where n is the number of commits between two bisect points. If we had twice as many commits per time unit, that'd only cause one additional step in a bisect across the same time "distance". A bit of back of the napkin math tells me it'd increase the steps for a bisect between commits one year apart from 17 to 18 to give you an idea of the scale. Again, that's with an assumed double the number of commits/time that we currently have.

It's actually even less meaningful of a difference in the context of Nix because the case at hand is splitting the same amount of meaningful change into smaller units of change and it's only really the meaningful changes that incur any significant cost during a bisect because unsubstantial changes eval to the same result and are therefore cached.

And that's also ignoring useful tricks like first-parent bisect which makes git bisect scale with the number of PRs against master; decoupling it from the amount of commits per PR entirely.

As long as we're not talking about a quadratic increase in PR throughput, we don't need to worry about our ability to bisect any time soon.

IMO allowing humans to understand changes better is so much more important than keeping some number down that only affects automated tooling in a small way at worst.

Fine, there's no need to go that far to make your point, I know all of this. My concern about minimizing unnecessary commits is less about tooling like git bisect and more about keeping the repository as streamlined as possible for both humans and tools. While individual changes may seem trivial, the accumulation of these over time can still impact the overall clarity and maintainability of the repository.

Also, the point here is to have common good sense and where we set the minimum threshold to merge a PR.

I tend to make sure that every least single thing that can be improved needs to be done before merging, while you're advocating for doing it "later"(tm).

Noting nits is fine IMHO (opinions differ on this) but they should not block a meaningful change for no good reason. I agree that it'd be nice to modernise the derivation a bit but that's simply not the goal of this PR. If @cleverca22 is fine with the derivation as it was, I see no reason to disallow it.
What requires significant human resources are meaningful or significant changes. Tiny refactors such as the ones you suggested don't require much reviewer attention. If I saw a stand-alone PR only consisting of such changes and the CI was green, I'd hit the merge button and close the tab within seconds.

Another point where I disagree is regarding the goal of this PR. Whether it’s introducing a new derivation or reverting an old one, it should be up-to-date and correct in every aspect from the start.

If we don’t address these issues now, they’ll likely need to be fixed "later"(tm) in a new PR, which adds unnecessary commits, consumes resources, and requires more time and attention from reviewers. With the number of PRs already awaiting review, don’t you think it’s better to avoid creating more when it can be handled now?

I'm not sure what you're tying to tell me here. Are you implying that the ability to nitpick PRs to death is somehow critical for your involvement in Nixpkgs?

To be clear, I don’t see these as mere nitpicks. New contributors often look at PRs for guidance, and it’s important that they find good examples to follow.

FWIW I don't desire anyone leaving the project (even people I have trouble with on a personal level sometimes) but that also applies to contributors who do not have merge privileges.

I don’t have any personal issues with anyone, inclding you; my focus is solely on improving Nix and contributing to its success. While I respect your perspective, I simply disagree with your assessment and the way my points were dismissed just because you don't agree. And this is the reason why things like this, makes me want to put my free time in another project.

That said, our differing views won’t stop me from grabbing a drink with you if the opportunity ever arises.

If the typical contributing experience to Nixpkgs is that a gatekeeper blocks your simple PR because they want to see minor adjustments to the code to better fit their personal preferences in packages they don't even maintain that are unrelated to the core purpose of the PR, that's bad. Extremely bad IMHO.

I don’t see myself as a gatekeeper; my goal is to help contributors improve, which is, in my view, a key role of maintainers. If a contributor feels frustrated by receiving constructive feedback in an open-source project, I understand, but that’s part of the open-source process. We’ve all faced criticism at some point... it’s how we learn and grow as contributors. If someone isn’t open to feedback or improvement, they may struggle in this environment, as it’s an essential part of collaboration in open source.

This is not directed at you in particular btw, I've been absolutely guilty of this too. It's super easy to fall into this behaviour without realising it. I see the only way to change this unfortunate status quo is to call out this behaviour whenever I notice it happening.

I'm not suggesting that nitpicking is central to my involvement in nixpkgs. Rather, my level of involvement correlates with the project's alignment with values like code quality and maintainability. If the general direction leans toward prioritizing speed over these, I might naturally step back a bit. This isn’t a statement about leaving or diminishing anyone else's contributions but rather about maintaining the standards I aim for in my work.

That was my last comment in here, I think I made my point and I don't want to spend more time on this, it's too much already. Feel free to catch me on Matrix if you want to discuss this further.

@Atemu
Copy link
Member

Atemu commented Sep 16, 2024

Also, the point here is to have common good sense and where we set the minimum threshold to merge a PR.

I tend to make sure that every least single thing that can be improved needs to be done before merging, while you're advocating for doing it "later"(tm).

Like many of us, I too am a perfectionist. I also like to see this sort of thing happen but we need to recognise our perfectionism and take care to not impose it on others too much when we don't know whether they share it. As I mentioned, I think it's fine to suggest such improvements but I draw the line at forcing them onto others.

That's especially true if it's clear that the change isn't intended to be of an improving nature in any way. I wouldn't even think to suggest refactors in a simple version bump for instance and I don't think you would either. A revert is about as much of a code improvement as renaming a file. Expecting refactors is inappropriate in either case.

it should be up-to-date and correct in every aspect from the start.

While I too love each and every thing in Nixpkgs to be perfect, this is too much to impose onto those who don't care as much.

Also consider that this is code that was already in Nixpkgs. It's not ideal but more than acceptable and only things that are truly unacceptable should be blocked.

I simply disagree with your assessment and the way my points were dismissed just because you don't agree.

I don't do this because I disagree with your desire for improvement, I do this because speaking up against aggressive nitpicking the right thing to do.

We should all know by now that it's not exactly welcoming to have such changes imposed onto you. We've had numerous discussions on this. There's even a documented policy against nitpicking PRs to death now.

That said, our differing views won’t stop me from grabbing a drink with you if the opportunity ever arises.

Sure, I'll likely be at NixCon if you're going.

I don’t see myself as a gatekeeper; my goal is to help contributors improve, which is, in my view, a key role of maintainers.

The problem is that you are. I don't even want to know the amount of PRs that I have requested nitpicks on that then went silent and I forgot about them. That's how PRs die and you lose contributors.

Please answer honestly: Would this PR have been merged as-is if your nitpicks weren't resolved by the end of the week?

If the answer to that is "no" then you are indeed a gatekeeper.

As you mentioned, gatekeeping isn't strictly a bad thing: Truly unacceptable things should be kept away by someone assuming a gatekeeper role.
That should be reserved for things that actually hurt us though. A rec rather than finalAttrs or sha256 rather than hash do not hurt us in any significant way and there isn't even consensus on the former; that one is a matter of taste in many respects.

That was my last comment in here, I think I made my point and I don't want to spend more time on this, it's too much already. Feel free to catch me on Matrix if you want to discuss this further.

I read this when I had the rest of my reply written out...

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants