Skip to content

arm-trusted-firmware: set unfree only if hdcp.bin is used; otherwise delete it before building#174691

Merged
Mindavi merged 1 commit intomasterfrom
unknown repository
Jun 4, 2022
Merged

arm-trusted-firmware: set unfree only if hdcp.bin is used; otherwise delete it before building#174691
Mindavi merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented May 26, 2022

This minimizes the scope of a user-visible behavior change relative to 21.11, so it is worth at least considering backporting it before the release deadline. #172160

Description of changes

The unfreeIncludeHDCPBlob parameter for arm-trusted-firmware was introduced as a result of this reviewer request and ultimately merged as part of #158310. This was part of correcting the fact that, previously, arm-trusted-firmware had the wrong meta.license.

The default value unfreeIncludeHDCPBlob?true causes a change in the meta.license field for all of the subpackages within pkgs/misc/arm-trusted-firmware/, and results in them needing NIXPKGS_ALLOW_NONFREE=1.

For Rockchip platforms this change is unavoidable; we are correcting an incorrect license declaration.

For non-Rockchip platforms the file hdcp.bin does not get included in the output; the blob is for a Synopsys HDCP core that is currently used only by Rockchip (although other companies could license it from Synopsys in the future). Therefore on non-Rockchip we can delete hdcp.bin before building instead of changing the license. This preserves the ability to build them without NIXPKGS_ALLOW_NONFREE=1.

Let's do that.

Deleting hdcp.bin ensures that we won't be caught by surprise if some future non-Rockchip Arm CPU licenses the same Synopsys HDCP core that Rockchip is using.

Thanks to @samueldr for pointing out the overly-broad NIXPKGS_ALLOW_NONFREE=1 requirement.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from lopsided98 May 26, 2022 07:40
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 26, 2022
@Mindavi
Copy link
Contributor

Mindavi commented May 29, 2022

Isn't this possible to do with a single parameter? This looks like it may cause issues in the future due to the increased complexity.

I like doing this in general.

@ghost
Copy link
Author

ghost commented May 29, 2022

Isn't this possible to do with a single parameter?

There is only one top-level parameter visible outside of the file: unfreeIncludeHDCPBlob.

The other parameter platformCanUseHDCPBlob is not visible outside of arm-trusted-firmware.nix. The arm-trusted-firmware.nix file actually builds five platform-specific packages for five different CPUs. The platformCanUseHDCPBlob flag indicates which of these CPUs can delete hdcp.bin (without any change in functionality) rather than having to mark the package as unfree.

Perhaps I missed some other strategy here?

@ghost
Copy link
Author

ghost commented May 29, 2022

The other parameter platformCanUseHDCPBlob is not visible outside of arm-trusted-firmware.nix.

Okay that is not entirely true.

The new parameter is not exposed to users of the armTrustedFirmware${PLATFORM} packages.

However it is exposed to users who call buildArmTrustedFirmware directly. For example, somebody writing a nixpkgs expression for a new ARM CPU which isn't yet included in arm-trusted-firmware.nix. But for those users it should be exposed so they can set it correctly: only they know if the new CPU uses hdcp.bin or not.

So I do think we actually need two parameters here. platformCanUseHDCPBlob is an objective fact about the CPU in question whereas unfreeIncludeHDCPBlob is a user preference.

@ghost
Copy link
Author

ghost commented May 29, 2022

Added a comment to explain the new parameter, and reordered the args list to place it next to platform. No other changes.

@ghost ghost changed the title arm-trusted-firmware: set unfreeIncludeHDCPBlob=false if not used arm-trusted-firmware: set unfree only if hdcp.bin is used; otherwise delete it before building May 29, 2022
@github-actions github-actions bot added 6.topic: emacs Text editor 6.topic: kernel The Linux kernel 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: python Python is a high-level, general-purpose programming language. 8.has: module (update) This PR changes an existing module in `nixos/` labels May 29, 2022
@ghost
Copy link
Author

ghost commented May 29, 2022

Latest push just squashes to a single commit, no other changes.

In a moment I will submit another PR with an alternative implementation that might do a better job of addressing @Mindavi's concern.

@ghost ghost requested review from Mic92, hedning, jtojnar, kalbasit and zowoq as code owners May 29, 2022 22:52
@github-actions github-actions bot removed 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. labels May 29, 2022
@ghost
Copy link
Author

ghost commented May 29, 2022

In a moment I will submit another PR with an alternative implementation that might do a better job of addressing @Mindavi's concern.

#175372

I have verified that the top-level expressions are extensionally equivalent -- they nix-instantiate to the same derivation for both this PR and #175372.

The `unfreeIncludeHDCPBlob` parameter was introduced as a result of
this reviewer request:

  #148890 (comment)

The default value `unfreeIncludeHDCPBlob?true` causes a change in the
`meta.license` field for all of the subpackages within
`pkgs/misc/arm-trusted-firmware/`, and results in them needing
`NIXPKGS_ALLOW_NONFREE=1`.

For non-Rockchip platforms the file hdcp.bin does not get included in
the output; the blob is for a Synopsys HDCP core that is currently
used only by Rockchip (although other companies could license it from
Synopsys in the future). Therefore on non-Rockchip we can delete
hdcp.bin before building instead of changing the license. This
preserves the ability to build them without NIXPKGS_ALLOW_NONFREE=1.

Let's do that.

Deleting hdcp.bin ensures that we won't be caught by surprise if some
future non-Rockchip Arm CPU licenses the same Synopsys HDCP core that
Rockchip is using.

Use easier-to-follow names for controlling the blob
inclusion/exclusion.  Also, if the blob is believed to be unnecessary,
delete it beforehand so we will know if we were wrong about that belief.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@ofborg ofborg bot removed request for a team, jonringer and zowoq May 29, 2022 23:07
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 29, 2022
@mweinelt
Copy link
Member

You're pinging people left and right, please be more mindful about what you force-push.

@mweinelt mweinelt removed their request for review May 30, 2022 00:20
@ghost
Copy link
Author

ghost commented May 30, 2022

You're pinging people left and right

I know. I don't want to send these pings. Please tell me how to disable or retract the pings.

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Ok yeah, I do like this one more, I guess.

Ofborg results also seem ok, it indeed refuses to build the rk3399 but does build the normal allwinner one.

@Mindavi
Copy link
Contributor

Mindavi commented May 30, 2022

@ofborg build armTrustedFirmwareRK3399 armTrustedFirmwareAllwinner

@ghost
Copy link
Author

ghost commented May 31, 2022

Actually you need to

@ofborg build pkgsCross.aarch64-multiplatform.armTrustedFirmwareRK3399

I think ofborg runs with NIXPKGS_ALLOW_UNFREE=1.

@ghost
Copy link
Author

ghost commented May 31, 2022

For completeness, to cover the platformCanUseHDCPBlob=false case:

@ofborg build pkgsCross.aarch64-multiplatform.armTrustedFirmwareAllwinner

And the buildPlatform-side tooling, which has a somewhat-different build path:

@ofborg build pkgsCross.armTrustedFirmwareTools

That should cover the important stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants