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

ubootTeresA64: init #241360

Closed
wants to merge 3 commits into from
Closed

ubootTeresA64: init #241360

wants to merge 3 commits into from

Conversation

Kreyren
Copy link
Contributor

@Kreyren Kreyren commented Jul 3, 2023

Description of changes

This merge request will add an official support of NixOS for the Open-Source Hardware+Software OLIMEX Teres-A64 through integrating U-Boot with crust for the device.

Requires: #241358

Things done
  • Built on platform(s)
    • x86_64-linux
    • [NOT TESTED] aarch64-linux
    • [UNABLE] x86_64-darwin
    • [UNABLE] aarch64-darwin
  • [IRELEVANT] For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • [ASKED_NOT_TO] (Package updates) Added a release notes entry if the change is major or breaking
    • [IRELEVANT] (Module updates) Added a release notes entry if the change is significant
    • [IRELEVANT] (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.
  • Custom
Additional notes

or1k-none cannot be set as a binfmt:

error: Cannot create binfmt registration for system or1k-none

So i decided to use the crossPkgs definition for SCP in uboot

Wikipage

I've created a wikipage for the device on https://nixos.wiki/wiki/NixOS_on_ARM/OLIMEX_Teres-A64

@Kreyren Kreyren changed the title crustTeresA64: Init OLIMEX Teres-A64: Official Support Jul 3, 2023
.github/CODEOWNERS Outdated Show resolved Hide resolved
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

I have only reviewed the U-Boot bits.

It's not good.

The commit messages are not good, the work is not finished, this was made on top of ffabe30, which explains the multiple merge conflicts; a commit from 2021.

Furthermore, the commit for U-Boot changes is broadly incorrect: it should have been a few set of discrete commits with good messages indicating why the changes were made.

It's highly likely I missed other issues.


In addition to all that...

The gall of asking for kleptocurrency donations on a PR that should have been trivial, is really beyond my comprehension. Just for that I was tempted to close the PR with the comment "no.".

This is not how things are around here. This is not how things should be around here.

You will need to work on your attitude (related to matrix room behaviour) and clean up your act and how the contribution is authored to the highest standard for this to get accepted. This is not a good look. In addition to the bad vibes I got from going to your GitHub profile.

pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
Comment on lines 1 to 6
###! # U-Boot
###! Bootloader for Embedded boards based on PowerPC, ARM, MIPS and several other processors
###!
###! # Note on aarch64 sunxi devices
###! These devices require BL31 (Trusted Arm Firmware) and SCP (Power Manager) declared as environmental variables that point to the required file for the build to succeed and work without issues, refer to https://u-boot.readthedocs.io/en/latest/board/allwinner/sunxi.html for details

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this change was needed?

  • What's ###! all about?
  • We don't add package descriptions like that, it's lost into nothingness, the comment is not used at any place. We have a place for that, it's meta.description.
  • That note about Allwinner, while meaning well, is not at the correct place. And also is kinda wrong. It should be in the Nixpkgs manual, in a (new) section about packaging U-Boot, if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why this change was needed? -- @samueldr (#241360 (comment))

I like to include documentation with helpful informations so that the next guy has easier time with the codebase.

What's ###! all about? -- @samueldr (#241360 (comment))

Convention taken from rust as it seems that supermajority of NixOS developers have some experience with the language so i though that doing it this way will make it understandable for it's purpose.

We don't add package descriptions like that, it's lost into nothingness, the comment is not used at any place. We have a place for that, it's meta.description.

Elaborate?

That note about Allwinner, while meaning well, is not at the correct place. And also is kinda wrong. It should be in the Nixpkgs manual, in a (new) section about packaging U-Boot, if needed. -- @samueldr (#241360 (comment))

The convention that i use is to include short and meaningful in-code docs and if needs be put details in manual (or ideally generate the manual from the in-code docs)

pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Show resolved Hide resolved
@Kreyren Kreyren changed the title OLIMEX Teres-A64: Official Support OLIMEX Teres-A64: Semi-Official Support Jul 4, 2023
@Kreyren Kreyren changed the title OLIMEX Teres-A64: Semi-Official Support OLIMEX Teres-A64: Bootloader Support Jul 4, 2023
@Kreyren Kreyren changed the title OLIMEX Teres-A64: Bootloader Support OLIMEX Teres-A64: Bootloader Compatibility Jul 4, 2023
@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 4, 2023

The commit messages are not good, the work is not finished, this was made on top of ffabe30, which explains the multiple merge conflicts; a commit from 2021 -- @samueldr (#241360 (review))

Sorry didn't notice it, will fix (migrating from guix where i am used to working in emacs, but that is not comfortable for me to use with nixlang so i am getting used to vscodium again and was using the built-in git and it apparently doesn't synced the branch correctly, should be fixed now..)

rest discussed over matrix

pkgs/misc/crust/default.nix Outdated Show resolved Hide resolved
pkgs/misc/crust/default.nix Show resolved Hide resolved
pkgs/misc/crust/default.nix Show resolved Hide resolved
To maintain OLIMEX Teres-A64 and things tha tinterest me
@Kreyren Kreyren closed this Jul 5, 2023
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

Note that there will be a lot of inherit added in suggestions. You can and should use a single inherit x y z; instead of the individual suggestions.

I'd also have liked a response to the questions from the previous review, rather than a silent "resolve" action. There were reasons behind the questions.

You should review your commit messages for typos.

Also rename the PR to better follow the usual contribution guidelines; e.g. here the intention is adding ubootTeresA64, the other parts are work toward this.

ubootTeresA64: init

That should be the PR title, as this is what will be shown when going through commits. The PR title means nothing. What is "bootloader compatibility"? Is this adding GRUB support? EDKII? some other bootloader? No, it's U-Boot. Using the package name is the norm.

Also you should review carefully your PR body. What is “official support”, who made that decision? Did anything else change?


I'll re-add that note from my initial review. It still applies.

You will need to work on your attitude (related to matrix room behaviour) and clean up your act and how the contribution is authored to the highest standard for this to get accepted. This is not a good look. In addition to the bad vibes I got from going to your GitHub profile.

Comment on lines +40 to +43
# Strip "_defconfig" suffix from 'defconfig' if it's present for clearer output
# builtins.match returns 'Null' if the string matches so we have to then check if it's Null
# FIXME-QA(Krey): I hate this, but am not aware of better way to write this
pname = if (builtins.isNull ((builtins.match "_defconfig$" defconfig)))
then "crust-${lib.strings.removeSuffix "_defconfig" defconfig}"
else "crust-${defconfig}";
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the intent here.

Your comment only describes what the thing is doing. Not what the intention behind doing the thing is.

What are the situations where _defconfig would not be present?

Also: FIXME-QA, what? if you meant to ask questions about this, I'm not aware you have.

Anyway, pick a lane and stick to it, either it always has _defconfig or never has it. Coding "defensively" in a declarative environment is not useful, and only adds noise.

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 don't understand the intent here. -- @samueldr (#241360 (comment))

The intent is removing the _defconfig suffix for clearer nix output

What are the situations where _defconfig would not be present? -- @samueldr (#241360 (comment))

When building non-standard configuration e.g. non-standard teres forks and i overall i like to make my code as flexible as possible to allow for future expansions.

Also: FIXME-QA, what? if you meant to ask questions about this, I'm not aware you have. -- @samueldr (#241360 (comment))

Those are code indexing comments after self-review as i am not happy with the declaration of:

(builtins.isNull ((builtins.match "_defconfig$" defconfig)))

As i find it hard to read and it's doing builtins.match for regex suffix of _defconfig$ which returns Null (would argue for adjusting the buildins.match returns) which then has to be processed with builtins.isNull to get a Boolean for if conditional.

I prefer to write code so that it's easily readable e.g.

# In case `$pname` ends with `_defconfig` -> set pname without _defconfig
case "$pname" in *"_defconfig") pname="${pname//_defconfig/}"; esac

So that the code can be read in plain english by those who understand it's syntax comparing to:

# if match of '_defconfig$' from defconfig is null then ...
if (builtins.isNull ((builtins.match "_defconfig$" defconfig))) ...

Anyway, pick a lane and stick to it, either it always has _defconfig or never has it. Coding "defensively" in a declarative environment is not useful, and only adds noise. -- @samueldr (#241360 (comment))

It's not defensive coding it has a functional use as explained above.

crustTeresA64 = buildCrust rec {
defconfig = "teres_i_defconfig";
extraMeta = {
description = "SCP (power management) firmware for the odrysian king";
Copy link
Member

Choose a reason for hiding this comment

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

Don't try to be funny in the description. The description field has to be useful.

Generally using what's in the "about" section of the project is used, so following

We'd use SCP (power management) firmware for sunxi SoCs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't try to be funny in the description. The description field has to be useful. -- @samueldr (#241360 (comment))

It's not trying to be funny it's branding as the device is named after the king of the Odrysian state of Thrace where Plovdiv is now located (vendor's location).

We'd use SCP (power management) firmware for sunxi SoCs. -- @samueldr (#241360 (comment))

In case the rationale above is not sufficient i would do:

SCP (Power Management) firmware for OLIMEX Teres-A64

Comment on lines +1 to +9
###! Libre Power Management firmware for sunxi System On Chip ("SoC") for implementing system suspend/resume, and (onboards without a PMIC) soft poweroff/on. The package has to be built using (OpenRISC 1000, *not RISC-V*) compiler which is officially supported in upstream GCC starting with GCC 9.1.0.
###!
###! To build only the crust firmware derivation use:
###! $ nix-build -A pkgsCross.or1k.crustTeresA64
###!
###! When packaging for linux make sure that you have the following patches otherwise crust won't be able to cleanly share the clock controller and PMIC bus controller hardware: https://github.com/torvalds/linux/compare/master...crust-firmware:linux:crust
###!
###! If you are faced with issues while building the firmware then take a look at https://github.com/crust-firmware/crust#building-the-firmware

Copy link
Member

Choose a reason for hiding this comment

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

We don't really put package metadata as comments.

Also, what's up with ###!. This is not a convention from Nixpkgs. You never ended-up answering in the comment I left previously.

We don't usually put such things in the package files as comments, as they're not used in any form; not part of the docs, nothing.

  • The note about the linux kernel is not needed. It's part of the project documentation. We expect people implementing things to read the documentation from the project, and at best, this comment will simply rot and become outdated.
  • The description is not needed, it's in meta.description that it should live.

The tip about building the package really straddles the fence on being useful or not. On the one hand, this is nothing special to this package, this is basic pkgsCross usage, but on the other hand not everyone really knows about pkgsCross and its proper usage, so that one could stay... but really better documentation of pkgsCross overall would be better.

Suggested change
###! Libre Power Management firmware for sunxi System On Chip ("SoC") for implementing system suspend/resume, and (onboards without a PMIC) soft poweroff/on. The package has to be built using (OpenRISC 1000, *not RISC-V*) compiler which is officially supported in upstream GCC starting with GCC 9.1.0.
###!
###! To build only the crust firmware derivation use:
###! $ nix-build -A pkgsCross.or1k.crustTeresA64
###!
###! When packaging for linux make sure that you have the following patches otherwise crust won't be able to cleanly share the clock controller and PMIC bus controller hardware: https://github.com/torvalds/linux/compare/master...crust-firmware:linux:crust
###!
###! If you are faced with issues while building the firmware then take a look at https://github.com/crust-firmware/crust#building-the-firmware
# To build crust packages use `pkgsCross.or1k`, e.g. for `crustTeresA64`:
# $ nix-build -A pkgsCross.or1k.crustTeresA64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really put package metadata as comments. -- @samueldr (#241360 (comment))

It's not metadata those are software specification comments (summary of what it is, how to build it, relevant notes for packaging and where to seek help)

Also, what's up with ###!. This is not a convention from Nixpkgs. You never ended-up answering in #241360 (comment). -- @samueldr (#241360 (comment))

It's answered on my end, though i am unable to link to it:

image

the ###! is meant to be a documenting comment as a clear separation from regular comment to define the scope of the information provided.

I am not aware of a nix lang's convension for this so i used ###! on recommendation considering that this is from rust which majority of nix developers have experience with to understand it's purpose which is software specification as that's practice from writting mission critical software as the device is my daily and needs to be reliable, it's to ensure that the code is not overcomplicated, robust and does what it's designed to do.

The note about the linux kernel is not needed. It's part of the project documentation. We expect people implementing things to read the documentation from the project, and at best, this comment will simply rot and become outdated. -- @samueldr (#241360 (comment))

The tip about building the package really straddles the fence on being useful or not. On the one hand, this is nothing special to this package, this is basic pkgsCross usage, but on the other hand not everyone really knows about pkgsCross and its proper usage, so that one could stay... but really better documentation of pkgsCross overall would be better. -- @samueldr (#241360 (comment))

It was included as i wasn't able to get a good answer on how to integrate this from the nix community and i expect nix-unexperienced users working with the code.

I am not qualified to make documentation on pkgsCross atm.

Comment on lines 29 to 37
buildCrust = {
version ? "${defaultVersion}"
, src ? null
, filesToInstall
, installDir ? "$out"
, defconfig
, extraConfig ? ""
, extraPatches ? []
, extraMakeFlags ? []
, extraMeta ? {}
, ... } @ args: stdenv.mkDerivation ({
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all this boilerplate? Most likely no.

Crust is a very simple and precise project, not like TF-A and U-Boot, where those will act wildly differently when built for other platforms.

We only need one input, and one output. The defconfig name (with or without _defconfig, but not either at once), and the output is always scp.bin whichever the input is.

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 believe this was explained above, the declaration is made to be flexible to accompany the nature of OSHW devices like teres (people mod them and make custom PCBs so this flexibility is important).

Comment on lines +46 to +55

###! The package has upstream definitions of configurations for set devices and thus the defconfig has to be always included, beyond that the options are designed to be as flexible as possible to accompany special requirements of various devices
###!
###! Avoid declaring extra configuration (extraConfig) when possible as those should go to the upstream unless you use special usecase device e.g. modded teres-A64

defconfig = if defconfig == null then
throw "crust config not defined!"
else defconfig;

Copy link
Member

Choose a reason for hiding this comment

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

This can be removed entirely and replaced with an inherit.

This situation will never really happen unless someone passes defconfig = null to the function. And in that case, it will break for other reasons, and be an obvious fault.

Checking for null here does nothing useful.

Suggested change
###! The package has upstream definitions of configurations for set devices and thus the defconfig has to be always included, beyond that the options are designed to be as flexible as possible to accompany special requirements of various devices
###!
###! Avoid declaring extra configuration (extraConfig) when possible as those should go to the upstream unless you use special usecase device e.g. modded teres-A64
defconfig = if defconfig == null then
throw "crust config not defined!"
else defconfig;
inherit defconfig;

defconfig = "teres_i_defconfig";
extraMeta = {
description = "SCP (power management) firmware for the odrysian king";
platforms = [ "or1k-none" ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = [ "or1k-none" ];

Put this in buildCrust. Crust only supports that one architecture. If it ever grows to support more, this whole thing will require a whole revisit anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crust supports multiple triplets depending on the configuration such as the or1k-linux-musl- for pine64_plus_defconfig..

So the platform declaration is important to be kept as device independent as enforcing or1k-none package-wide will likely cause issues in scenarios of builds not well optimized for glibc (such as requirement for PIE hardening).

pkgs/misc/crust/default.nix Outdated Show resolved Hide resolved
Comment on lines +556 to +559
# U-Boot requires crust built using OpenRISC 1000 compiler for power management
SCP = if stdenv.hostPlatform.isOr1k
then "${crustTeresA64}/scp.bin"
else "${pkgsCross.or1k.crustTeresA64}/scp.bin";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# U-Boot requires crust built using OpenRISC 1000 compiler for power management
SCP = if stdenv.hostPlatform.isOr1k
then "${crustTeresA64}/scp.bin"
else "${pkgsCross.or1k.crustTeresA64}/scp.bin";
SCP = "${pkgsCross.or1k.crustTeresA64}/scp.bin";

The situation where this will not be pkgsCross.or1k will never happen. Plainly and simply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will happen in a scenarios where the host is able to build OpenRISC 1000 natively without cross compiling, some RISCV processors are reportedly able to do this and there is an ongoing work on RISCV for nixos.

The declaration was added there upon peer-review.

@@ -20,6 +20,8 @@
, armTrustedFirmwareRK3328
, armTrustedFirmwareRK3399
, armTrustedFirmwareS905
, crustTeresA64
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
, crustTeresA64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Elaborate?

@@ -543,6 +545,21 @@ in {
filesToInstall = ["u-boot-sunxi-with-spl.bin"];
};

# Refer to https://nixos.wiki/wiki/NixOS_on_ARM/OLIMEX_Teres-A64
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Refer to https://nixos.wiki/wiki/NixOS_on_ARM/OLIMEX_Teres-A64

This will not be useful to anyone. It's almost guaranteed. And this is not an official NixOS property, so linking it as an authority here isn't exactly right either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a wikipage that i created for this package so that it has relevant informations on packaging as including that in-code seemed too distracting to me.

Do you want me to create a summary in the commant?

Comment on lines 567 to 552
# FIXME(Krey): Figure out how to build this with crust
#SCP = "${crustTeresA64}/scp.bin";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

HELP_WANTED: I don't know how to make nix to build crust on OpenRISC (builds separately fine), take it's binary and use it to build the U-Boot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved in next commit(s)

pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
pkgs/misc/uboot/default.nix Outdated Show resolved Hide resolved
Comment on lines 1 to 6
###! # U-Boot
###! Bootloader for Embedded boards based on PowerPC, ARM, MIPS and several other processors
###!
###! # Note on aarch64 sunxi devices
###! These devices require BL31 (Trusted Arm Firmware) and SCP (Power Manager) declared as environmental variables that point to the required file for the build to succeed and work without issues, refer to https://u-boot.readthedocs.io/en/latest/board/allwinner/sunxi.html for details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain why this change was needed? -- @samueldr (#241360 (comment))

I like to include documentation with helpful informations so that the next guy has easier time with the codebase.

What's ###! all about? -- @samueldr (#241360 (comment))

Convention taken from rust as it seems that supermajority of NixOS developers have some experience with the language so i though that doing it this way will make it understandable for it's purpose.

We don't add package descriptions like that, it's lost into nothingness, the comment is not used at any place. We have a place for that, it's meta.description.

Elaborate?

That note about Allwinner, while meaning well, is not at the correct place. And also is kinda wrong. It should be in the Nixpkgs manual, in a (new) section about packaging U-Boot, if needed. -- @samueldr (#241360 (comment))

The convention that i use is to include short and meaningful in-code docs and if needs be put details in manual (or ideally generate the manual from the in-code docs)

defconfig = "teres_i_defconfig";
extraMeta = {
description = "SCP (power management) firmware for the odrysian king";
platforms = [ "or1k-none" ];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Crust supports multiple triplets depending on the configuration such as the or1k-linux-musl- for pine64_plus_defconfig..

So the platform declaration is important to be kept as device independent as enforcing or1k-none package-wide will likely cause issues in scenarios of builds not well optimized for glibc (such as requirement for PIE hardening).

Comment on lines +556 to +559
# U-Boot requires crust built using OpenRISC 1000 compiler for power management
SCP = if stdenv.hostPlatform.isOr1k
then "${crustTeresA64}/scp.bin"
else "${pkgsCross.or1k.crustTeresA64}/scp.bin";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will happen in a scenarios where the host is able to build OpenRISC 1000 natively without cross compiling, some RISCV processors are reportedly able to do this and there is an ongoing work on RISCV for nixos.

The declaration was added there upon peer-review.

@@ -543,6 +545,21 @@ in {
filesToInstall = ["u-boot-sunxi-with-spl.bin"];
};

# Refer to https://nixos.wiki/wiki/NixOS_on_ARM/OLIMEX_Teres-A64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a wikipage that i created for this package so that it has relevant informations on packaging as including that in-code seemed too distracting to me.

Do you want me to create a summary in the commant?

pkgs/misc/crust/default.nix Outdated Show resolved Hide resolved
Comment on lines +40 to +43
# Strip "_defconfig" suffix from 'defconfig' if it's present for clearer output
# builtins.match returns 'Null' if the string matches so we have to then check if it's Null
# FIXME-QA(Krey): I hate this, but am not aware of better way to write this
pname = if (builtins.isNull ((builtins.match "_defconfig$" defconfig)))
then "crust-${lib.strings.removeSuffix "_defconfig" defconfig}"
else "crust-${defconfig}";
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 don't understand the intent here. -- @samueldr (#241360 (comment))

The intent is removing the _defconfig suffix for clearer nix output

What are the situations where _defconfig would not be present? -- @samueldr (#241360 (comment))

When building non-standard configuration e.g. non-standard teres forks and i overall i like to make my code as flexible as possible to allow for future expansions.

Also: FIXME-QA, what? if you meant to ask questions about this, I'm not aware you have. -- @samueldr (#241360 (comment))

Those are code indexing comments after self-review as i am not happy with the declaration of:

(builtins.isNull ((builtins.match "_defconfig$" defconfig)))

As i find it hard to read and it's doing builtins.match for regex suffix of _defconfig$ which returns Null (would argue for adjusting the buildins.match returns) which then has to be processed with builtins.isNull to get a Boolean for if conditional.

I prefer to write code so that it's easily readable e.g.

# In case `$pname` ends with `_defconfig` -> set pname without _defconfig
case "$pname" in *"_defconfig") pname="${pname//_defconfig/}"; esac

So that the code can be read in plain english by those who understand it's syntax comparing to:

# if match of '_defconfig$' from defconfig is null then ...
if (builtins.isNull ((builtins.match "_defconfig$" defconfig))) ...

Anyway, pick a lane and stick to it, either it always has _defconfig or never has it. Coding "defensively" in a declarative environment is not useful, and only adds noise. -- @samueldr (#241360 (comment))

It's not defensive coding it has a functional use as explained above.

@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 6, 2023

@samueldr I've made changes, please review (github button doesn't work for me)

Jacob Hrbek added 2 commits July 6, 2023 13:34
Dependency for U-Boot to make functional Power Management
Adding bootloader compatibility for the device
@samueldr
Copy link
Member

samueldr commented Jul 6, 2023

Closing as the author is clearly unwilling to collaborate with Nixpkgs contributors, maintainers and collaborators. After being asked to “work on [their] attitude [...] and clean up [their] act and how the contribution is authored to the highest standard”, I have to rely on my judgement and say there is not much effort on that here.

When a project member asks you to do something, you do not answer and justify with a word salad and keep the thing as-it-is. If you believe that project member is in the wrong, ask for more clarifications. Putting your foot down on trivialities is not the right attitude for collaboration.

List of reasons some changes were requested:

  • Factually wrong understanding of how Nix and Nixpkgs work.
  • Using "specifications" that are used nowhere else in Nixpkgs.
  • Making some things more complicated than they need to be.

Pretty much everything falls into these categories.

I have been operating on the assumption there was good faith from the other party. I still think there is good faith, but there needs to be introspection about the behaviour and how to collaborate with people from the project before accepting contributions here.


Note to other Nixpkgs contributors: feel free to discuss with me about this whole situation. It's more involved than strictly just this PR.

@samueldr samueldr closed this Jul 6, 2023
@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 7, 2023

Replying to @samueldr (#241360 (comment))

I re-submitted the contribution strictly to your instructions, but i will not maintain and take limited responsibility as i see it as sacrificing on the code quality and limiting the use of teres on nixos for reasons explained.

if desired i will use this approach for all future contributions and include my version in 3rd party channel.

@Kreyren Kreyren changed the title OLIMEX Teres-A64: Bootloader Compatibility ubootTeresA64: init Jul 7, 2023
@Kreyren
Copy link
Contributor Author

Kreyren commented Jul 7, 2023

I'd also have liked a response to the questions from the previous review, rather than a silent "resolve" action. There were reasons behind the questions. -- @samueldr (#241360 (review))

There were issues with GitHub's processing of the replies, it was reported and should be now fixed. Let me know if you see the replies

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

Successfully merging this pull request may close these issues.

None yet

4 participants