#487 Add package hash checking to the buildgcc script downloaded by ./download crossgcc

Merged
and_who merged 1 commits from SolidHal/master into libreboot/master 1 year ago
SolidHal commented 1 year ago

Buildgcc version downloaded by ./download does not check the hashes of packages #471

Since the script downloads a specific (old) version of crossgcc, this patch was backported from https://github.com/coreboot/coreboot/commit/92d483a8927066ef16603023be05ad1fb7dea644

Buildgcc version downloaded by ./download does not check the hashes of packages https://notabug.org/libreboot/libreboot/issues/471 Since the script downloads a specific (old) version of crossgcc, this patch was backported from https://github.com/coreboot/coreboot/commit/92d483a8927066ef16603023be05ad1fb7dea644
Leah Rowe commented 1 year ago
Owner

this is being rejected, since it's a modification to the old build system which will be dumped upon the next release.

please check whether the same bug exists in the new build system, and fix it there.

this is being rejected, since it's a modification to the old build system which will be dumped upon the next release. please check whether the same bug exists in the new build system, and fix it there.
Leah Rowe commented 1 year ago
Owner

anything under resources/ is deprecated

anything under resources/ is deprecated
SolidHal commented 1 year ago
Poster

this is being rejected, since it's a modification to the old build system > which will be dumped upon the next release.

Understandable, I was not aware.

please check whether the same bug exists in the new build system, and fix > it there.

This issue is fixed in the new build system. Would a warning or note of this known issue be appropriate to dissuade people from using the old build system?

> this is being rejected, since it's a modification to the old build system > which will be dumped upon the next release. Understandable, I was not aware. > please check whether the same bug exists in the new build system, and fix > it there. This issue is fixed in the new build system. Would a warning or note of this known issue be appropriate to dissuade people from using the old build system?
Andrew Robbins commented 1 year ago
Collaborator

@vimuser, I have no issue with patching the old build system with security fixes until the new build system is operational. I would consider this important enough to allow through so I'm reopening this pull request temporarily.

@vimuser, I have no issue with patching the old build system with security fixes until the new build system is operational. I would consider this important enough to allow through so I'm reopening this pull request temporarily.

Considering that the old build system is currently the only way to build libreboot (Correct me if I'm wrong) since my commits a month or so ago, It's probably a good idea to merge this.

Considering that the old build system is currently the only way to build libreboot (Correct me if I'm wrong) since my commits a month or so ago, It's probably a good idea to merge this.
Andrew Robbins commented 1 year ago
Collaborator

@SolidHal, I'll merge your commit provided you alter the commit message to include the original author's name, like so:

Jonathan Neuschäfer <j.neuschaefer@gmx.net>

In addition to making a change proposed by @swiftgeek in #libreboot to do

archive="$package"_ARCHIVE
archive="${!archive}"

instead of this (wherever it occurs):

archive="$(eval echo \$$package"_ARCHIVE")"

The reasoning for the latter change is that it makes it more clear as to what's going on there with that variable assignment. You can see a good description of this at http://tldp.org/LDP/abs/html/bashver2.html#EX78

Also, please do not include 'signed off by' lines in your commit message--Libreboot does not use these, unlike Coreboot.

@SolidHal, I'll merge your commit provided you alter the commit message to include the original author's name, like so: Jonathan Neuschäfer \<j.neuschaefer@gmx.net\> In addition to making a change proposed by @swiftgeek in `#libreboot` to do ``` archive="$package"_ARCHIVE archive="${!archive}" ``` instead of this (wherever it occurs): ``` archive="$(eval echo \$$package"_ARCHIVE")" ``` The reasoning for the latter change is that it makes it more clear as to what's going on there with that variable assignment. You can see a good description of this at http://tldp.org/LDP/abs/html/bashver2.html#EX78 Also, please do not include 'signed off by' lines in your commit message--Libreboot does not use these, unlike Coreboot.
SolidHal commented 1 year ago
Poster

@and_who, should I expand the patch to replace every instance of

archive="$(eval echo \$$package"_ARCHIVE")"

in buildgcc, or just the instances that this patch adds? I could see arguments for both options.

@and_who, should I expand the patch to replace every instance of ``` archive="$(eval echo \$$package"_ARCHIVE")" ``` in buildgcc, or just the instances that this patch adds? I could see arguments for both options.
Andrew Robbins commented 1 year ago
Collaborator

@SolidHal, replace every indirect variable reference in the buildgcc file if you wouldn't mind.

@SolidHal, replace every indirect variable reference in the buildgcc file if you wouldn't mind.
SolidHal commented 1 year ago
Poster

@and_who I've made the requested changes

@and_who I've made the requested changes
Andrew Robbins commented 1 year ago
Collaborator

@SolidHal, the only things you need to fix now are related to the commit message so it should be super simple to address:

  1. Remove the "with a patch" part of the commit message subject line (it's superfluous)
  2. Add an empty line after the subject line
  3. Use a Coreboot URI instead of Github
  4. The commit author date needs to be updated (this can be done by running git commit --amend --reset-author)

In the end your commit message should look like so:

Backport hash checking into buildgcc

Patch based off:
https://review.coreboot.org/cgit/coreboot.git/commit/?id=92d483a8927066ef16603023be05ad1fb7dea644
by Jonathan Neuschäfer <j.neuschaefer@gmx.net>

Once this is done I will merge your changes. Thanks!

@SolidHal, the only things you need to fix now are related to the commit message so it should be super simple to address: 1. Remove the "with a patch" part of the commit message subject line (it's superfluous) 2. Add an empty line after the subject line 3. Use a Coreboot URI instead of Github 4. The commit author date needs to be updated (this can be done by running `git commit --amend --reset-author`) In the end your commit message should look like so: ``` Backport hash checking into buildgcc Patch based off: https://review.coreboot.org/cgit/coreboot.git/commit/?id=92d483a8927066ef16603023be05ad1fb7dea644 by Jonathan Neuschäfer <j.neuschaefer@gmx.net> ``` Once this is done I will merge your changes. Thanks!
SolidHal commented 1 year ago
Poster

@and_who Review comments addressed, let me know if there's anything else.

@and_who Review comments addressed, let me know if there's anything else.
Andrew Robbins commented 1 year ago
Collaborator

@SolidHal, In making the changes to your commit message, the changes requested in https://notabug.org/libreboot/libreboot/pulls/487#issuecomment-9994 were reverted. Could you add those back in?

@SolidHal, In making the changes to your commit message, the changes requested in https://notabug.org/libreboot/libreboot/pulls/487#issuecomment-9994 were reverted. Could you add those back in?
SolidHal commented 1 year ago
Poster

@and_who Sorry about that, and thank for catching my stupid mistake! That's what I get for developing on two machines. Hopefully its now perfect.

@and_who Sorry about that, and thank for catching my stupid mistake! That's what I get for developing on two machines. Hopefully its now perfect.
Andrew Robbins commented 1 year ago
Collaborator

Thanks!

Thanks!
This pull request has been merged successfully!
Sign in to join this conversation.
Loading...
Cancel
Save
There is no content yet.