Skip to content

ectool: init at 4.9#66494

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

ectool: init at 4.9#66494
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Aug 11, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@worldofpeace
Copy link
Contributor

@GrahamcOfBorg eval

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 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. labels Aug 11, 2019
@aanderse
Copy link
Member

@GrahamcOfBorg build ectool

Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

I don't have anything to test this with, and I'm not familiar with coreboot... but LGTM 👍
@worldofpeace anything to add? It is @petabyteboy (read: quality 😄 ), so a merge is in order...?

EDIT: Somehow I did not notice arch64 failure... 😞 @petabyteboy want to take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about?

makeFlags = [ 
  "PREFIX=${placeholder "out"}"
  "INSTALL=install"
];

preBuild = ''
  cd util/ectool
'';

Then I think you can drop the custom installPhase, though upstream should really be using DESTDIR too

https://github.com/coreboot/coreboot/blob/master/util/ectool/Makefile#L38

Copy link
Author

Choose a reason for hiding this comment

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

I would like to stick to the pattern used in all other coreboot util packages for now, and refactor or merge them later.

@worldofpeace
Copy link
Contributor

Somehow I did not notice arch64 failure... 😞 @petabyteboy want to take a look?

I think the error,

ec.c:22:10: fatal error: sys/io.h: No such file or directory
 #include <sys/io.h>

typically means something needs porting. Didn't look at the source though.

Copy link
Contributor

Choose a reason for hiding this comment

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

If in the future another package is added for coreboot utils, it would probably be best if there was a package that was just all of them.

Copy link
Author

Choose a reason for hiding this comment

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

Very valid point, I will look into it in the future.

@ghost
Copy link
Author

ghost commented Aug 31, 2019

  • rebased
  • updated to 4.10
  • changed platforms to x86 only

@ghost
Copy link
Author

ghost commented Aug 31, 2019

Somehow I did not notice arch64 failure... disappointed @petabyteboy want to take a look?

I think the error,

ec.c:22:10: fatal error: sys/io.h: No such file or directory
 #include <sys/io.h>

typically means something needs porting. Didn't look at the source though.

From https://bugzilla.redhat.com/show_bug.cgi?id=1116162:

The sys/ headers are architecture and OS dependent. They do not exist across all targets and io.h in particular is intended for very lowl-level non-portable uses often in coordination with the kernel. The only targets that provide sys/io.h are x86*, Alpha, IA64, and 32-bit ARM. No other systems provide it.

So either way this has do be done in upstream glibc or coreboot.

@ghost
Copy link
Author

ghost commented Aug 31, 2019

I will replace this with a PR merging all coreboot util packages into one for easier maintenance in the future.

@ghost ghost closed this Aug 31, 2019
@ghost
Copy link
Author

ghost commented Aug 31, 2019

superseeded by #67836

@worldofpeace
Copy link
Contributor

I will replace this with a PR merging all coreboot util packages into one for easier maintenance in the future.

That's great @petabyteboy ✨

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 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. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants