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

python3Packages.wled: 0.4.4 -> 0.5.0 #126319

Merged
merged 2 commits into from Jun 14, 2021
Merged

Conversation

fabaff
Copy link
Member

@fabaff fabaff commented Jun 9, 2021

Motivation for this change

Update to latest upstream release 0.5.0

Change log: https://github.com/frenck/python-wled/releases/tag/v0.5.0

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 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.

@fabaff
Copy link
Member Author

fabaff commented Jun 9, 2021

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

3 packages built:
  • home-assistant
  • python38Packages.wled
  • python39Packages.wled

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 126319 at 1e73c70 run on aarch64-linux 1

3 packages built successfully:
  • home-assistant
  • python38Packages.wled
  • python39Packages.wled

@risicle
Copy link
Contributor

risicle commented Jun 9, 2021

nixpkgs-review happy, macos 10.15 & nixos x86_64

@mweinelt mweinelt moved this from To do to WIP in Home Assistant Jun 11, 2021
@frenck
Copy link

frenck commented Jun 12, 2021

As the author of this package I would request removal of this package from this repository.

Thank you for considering and honoring my request

@risicle
Copy link
Contributor

risicle commented Jun 12, 2021

@frenck
Copy link

frenck commented Jun 12, 2021

I know, it is respectfully asking. I hope you can respect the wishes of the open source author in this landscape.

Thank you for considering.

@risicle
Copy link
Contributor

risicle commented Jun 12, 2021

I think I should emphasize the degree to which we take seriously and the efforts we make in nixpkgs to ensure a package's correct operation, especially when we make tweaks to it. In most cases, we attempt to enable a package's tests in the build procedure, and any updates made to a package will trigger all depending packages to themselves be rebuilt and run their tests, which generally reveals true incompatibilities pretty quickly.

In fact, most users of Nix use it because they find things like pypi/pip to be too unreliable and Nix gives them guarantees about the correctness of the installed state of the package they requested that they don't otherwise get from other tools (e.g. pip does not run tests on the target system that might result in weirdness). More often than not, we're usually the ones reporting to upstream authors that they have subtle version incompatibilities that they didn't know about.

This is not cowboy stuff we're doing, though it might look like it sometimes.

@frenck
Copy link

frenck commented Jun 12, 2021

Thanks for your response, could you please honor my request? Thanks.

@blaggacao
Copy link
Contributor

blaggacao commented Jun 12, 2021

could you please honor my request?

It seems unclear wether that will ultimately happen, there are arguments in favor and against it being discussed in the various meta-channels. There is also currently no generally accepted policy for how to deal with such requests, nor who is to decide.

However this particular PR is about a version bump and I predict the consideration to merge it might be treated within that narrow scope.

(this is my own view & interpretation, but I hope it furthers understanding)

@lukegb
Copy link
Contributor

lukegb commented Jun 12, 2021

Locked for now. Please see #126677 (comment).

@NixOS NixOS locked as too heated and limited conversation to collaborators Jun 12, 2021
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

Marking as changes requested until above issues get resolved.

However, I would like to note that selectively choosing who can consume FOSS is very anti-FOSS. I (and assume most people) respect the view of a maintainer not wanting to support scenarios that he/she/they did not intend the software to be used (e.g. home-assistant's native installation methods).

I don't think anyone will object to you having an issue-template on your repo's which clearly states that you do not, and will not support issues outside of home-assistant's intended installation.

But what (nixpkgs) people are objecting, is that this package along with ambee, have been made a core dependency in home-assistant. So it's no longer an option for us to keep home-assistant up-to-date without shipping and updating your packages along with it. One option would be to drop your packages from home-assistant being part of core, and we will no longer need to achieve feature parity with home-assistant by including your packages.

If you would like for us to consume your pypi releases, then please ship tests in your sdist. The only reason why we are targeting your github is that your sdist's don't contain tests. And since nixpkgs breaks most (or all) assumptions in the name of reproducibility, we really need tests to validate that all assumptions are present during use of a package.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

other than that little remark

pkgs/development/python-modules/wled/default.nix Outdated Show resolved Hide resolved
@NixOS NixOS unlocked this conversation Jun 14, 2021
@frenck
Copy link

frenck commented Jun 14, 2021

But what (nixpkgs) people are objecting, is that this package along with ambee, have been made a core dependency in home-assistant. So it's no longer an option for us to keep home-assistant up-to-date without shipping and updating your packages along with it.

Unfortunately, your reasoning is untrue for a couple of reasons.

A) Home Assistant has the ability to automatically fetch dependencies. However, I understand that has been made impossible by this project as I understand. So, 🤷
B) There are hundreds of Home Assistant dependencies you guys don't ship at this moment. An active example of this is PR #126047, which adds another package of mine, which has been a dependency for 1,5 years already. Yet, you have been able to ship without.

But I don't have to tell you guys that, because if you open the Home Assistant package in this repository, it has been nicely commented in the meta file for each of the integrations listed.

Considering I'm locked out of other communications in this GitHub org, because all threads are locked as "too heated" (which personally, I'm not heated... My request remain simple), yet community members of this org (include board members of this org) have been raiding the Home Assistant forums (my own home / I'm a maintainer of FYI) and other resources to public shame me. This has been highly disrespectful and the relationship with this project is IMHO not restorable. And I hope you can understand I don't want my name to be associated with this project for that reason.

Considering the actions taken by members of this project, I would find it more than fitting and respectable to honor my request to remove my packages from your indexes.

Thank you for your consideration.

With kind regards,

Franck Nijhof

@siraben
Copy link
Member

siraben commented Jun 14, 2021

Result of nixpkgs-review pr 126319 run on x86_64-darwin 1

2 packages built:
  • python38Packages.wled
  • python39Packages.wled

Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot requested a review from mweinelt June 14, 2021 19:25
@frenck
Copy link

frenck commented Jun 14, 2021

PS this package is outdated.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jun 14, 2021

B) There are hundreds of Home Assistant dependencies you guys don't ship at this moment. An active example of this is PR #126047, which adds another package of mine, which has been a dependency for 1,5 years already. Yet, you have been able to ship without.

We are working on this and because of the amount of packages we won't be done for a while. Users of home-assistant on NixOS need to be aware of this and watch their logs closely.

Considering the actions taken by members of this project, I would find it more than fitting and respectable to honor my request to remove my packages from your indexes.

We have considered your request and came to the conclusion to include your packages anyway as we want to ship a as complete as possible home-assistant experience. We do not ask for official upstream support or plan to be an official distribution method right now or any time near the future.

have been raiding the Home Assistant forums (my own home / I'm a maintainer of FYI) and other resources to public shame me.

We do not intent to publicly or privately shame or harass anyone. This behavior is not acceptable and does not align with our community values. We try our best to be a welcoming community for everyone but we obviously do not achieve that all the time. I am sorry how this all went down and I honestly hope that the future will be better.

If you happen to get any support requests by NixOS please tell them to not report bugs to you but us instead. We will evaluate the bug and if we come to the conclusion that it is not the fault of the non standard NixOS packaging or outdated/mismatched libraries we will think about forwarding it upstream with a good error description or a PR if we could fix it ourselves.

Edit:

We are also continuously discussing this and there may or may not be posted an official statement in the next days.

@SuperSandro2000 SuperSandro2000 merged commit 8a362be into NixOS:master Jun 14, 2021
Home Assistant automation moved this from WIP to Done Jun 14, 2021
@frenck
Copy link

frenck commented Jun 14, 2021

We are working on this and because of the amount of packages we won't be done for a while. Users of home-assistant on NixOS need to be aware of this and watch their logs closely.

This is part of the issue I have with the way it is distributed by this project. It gives the user the idea of running Home Assistant, however, at this point, it is known not to be a full Home Assistant and the end-user may run into surprises. In the end, they will knock on the door of Home Assistant and the integration maintainers.

I am sorry how this all went down and I honestly hope that the future will be better.

Thank you for saying that. That at least makes me feel better about it.

@frenck frenck mentioned this pull request Jun 14, 2021
13 tasks
@juliosueiras
Copy link
Contributor

juliosueiras commented Jun 14, 2021

This is part of the issue I have with the way it is distributed by this project. It gives the user the idea of running Home Assistant, however, at this point, it is known not to be a full Home Assistant and the end-user may run into surprises. In the end, they will knock on the door of Home Assistant and the integration maintainers.

just want to point out, that for home-assistant when running in nixos, it need the user to define very detail of which component they want to use and config outline, since the main purpose being to be able to have full grasp of the system, and most nixos users(as far I am aware) know there will be surprises, since that is the nature of the tool/distro and generally will seek to fix it/troubleshoot themself

@lukegb
Copy link
Contributor

lukegb commented Jun 14, 2021

For my part: I'm sorry for the way I handled things too. I was attempting to consolidate discussion in a single place and avoid additional NixOS folks from piling on, but locking all of the issues/PRs wasn't particularly helpful and really only served the purpose of shutting frenck out of the discussion (and people continued to pile on anyway); I should have left at least one of the issues open.

(to @frenck : thanks for your contributions to Home Assistant, and to open source software in general; you're clearly a very talented software developer and we're very grateful for all your efforts in the home automation space and elsewhere)

We are working on this and because of the amount of packages we won't be done for a while. Users of home-assistant on NixOS need to be aware of this and watch their logs closely.

This is part of the issue I have with the way it is distributed by this project. It gives the user the idea of running Home Assistant, however, at this point, it is known not to be a full Home Assistant and the end-user may run into surprises. In the end, they will knock on the door of Home Assistant and the integration maintainers.

I was wondering about this: it turns out at the moment I think NixOS HA installs are reported as being HA Core, which is not correct since we don't meet the criteria in https://github.com/home-assistant/architecture/blob/master/adr/0016-home-assistant-core.md (we're not in a venv, we're using the distro-provided Python packages). We should figure out if we can set up HA in some way to explicitly mark it as unsupported by upstream (possibly by forcing https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/system_info.py#L41 to set installation_type to "NixOS Distribution: Unsupported by Home Assistant"?)

I think @mweinelt was looking at making our documentation more explicit as well that installations through NixOS configurations are unsupported by upstream as well, and should be considered an "unofficial method" per https://github.com/home-assistant/architecture/blob/master/adr/0012-define-supported-installation-method.md, but some in-product note would be useful too.

@frenck
Copy link

frenck commented Jun 14, 2021

For my part: I'm sorry for the way I handled things too.

❤️

We should figure out if we can set up HA in some way to explicitly mark it as unsupported by upstream (possibly by forcing home-assistant/core@dev/homeassistant/helpers/system_info.py#L41 to set installation_type to "NixOS Distribution: Unsupported by Home Assistant"?)

Hmmm... it should not report that. I think that that is something I'd figure out on the Home Assistant side of things as well. As it should clearly not show an installation that doesn't meet the Core installation requirements as the "Core" installation method.

@mweinelt
Copy link
Member

Actually, we are Unknown. Not sure if this is anything we should change, it's probably counter-productive for our setups to appear explicitly in their analytics.

image

@frenck
Copy link

frenck commented Jun 14, 2021

it's probably counter-productive for our setups to appear explicitly in their analytics.

Besides it being fully opt-in, that is actually filtered and handled, so that is not something to worry about.

Edit: Current behavior seems fine, so I would not recommend adjusting that source. It being "Unknown" seems fair and right.

@fabaff fabaff deleted the bump-wled branch June 18, 2021 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet