This repository has been archived by the owner. It is now read-only.

Add kernel updater script #625

Merged
merged 6 commits into from Oct 14, 2017

Conversation

Projects
None yet
4 participants
@ata2001
Member

ata2001 commented Sep 25, 2017

#478

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Sep 25, 2017

Member

It could assert, whether it is running on a supported device before mounting boot, but I'm not sure that is needed, since it will only get installed on the supported devices.

Member

ata2001 commented Sep 25, 2017

It could assert, whether it is running on a supported device before mounting boot, but I'm not sure that is needed, since it will only get installed on the supported devices.

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Sep 25, 2017

Member

Perhaps we should mount the boot partition automatically with the fstab like on most distros, that way it won't cause a problem on the N900, and this script could be simpler.

Member

ata2001 commented Sep 25, 2017

Perhaps we should mount the boot partition automatically with the fstab like on most distros, that way it won't cause a problem on the N900, and this script could be simpler.

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Sep 25, 2017

Member

Yeah, I would prefer if boot was mounted automatically. Otherwise, kernel updates will go wrong (typical mistake in Arch installations). How about making this as a second PR, which we review+merge first?

Member

ollieparanoid commented Sep 25, 2017

Yeah, I would prefer if boot was mounted automatically. Otherwise, kernel updates will go wrong (typical mistake in Arch installations). How about making this as a second PR, which we review+merge first?

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Sep 25, 2017

Member

@ollieparanoid Okay, I'll do that first.

Member

ata2001 commented Sep 25, 2017

@ollieparanoid Okay, I'll do that first.

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Sep 25, 2017

Member

@ollieparanoid @ata2001 I also agree to always mount the /boot partition automatically

Member

drebrez commented Sep 25, 2017

@ollieparanoid @ata2001 I also agree to always mount the /boot partition automatically

@ata2001 ata2001 removed the dont_merge_pr label Oct 1, 2017

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Oct 1, 2017

Member

Now we can continue to work on this. Made a commit, now it leaves mounting the boot partition to the fstab.

Member

ata2001 commented Oct 1, 2017

Now we can continue to work on this. Made a commit, now it leaves mounting the boot partition to the fstab.

@ollieparanoid

Code looks good and I like how you have enabled automatic shellchecking.

Also using ${var:?} is good, because it will abort the script there if the variable is empty and print a message.

I have noted that with that PR it gets obvious that our code only supports one value for deviceinfo_flash_methods, so I think we should change it to deviceinfo_flash_method and make it obvious in the documentation (in a new PR and after discussing this in #460).

Finally, I have not tested this on a device. Although I'm sure that you have already tested it, code becomes better when someone else tests it. So if you don't mind, I would wait with merging this PR until someone confirms that it is working on their device.

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Oct 6, 2017

Member

@ata2001 should I test the isorec use case?

Member

drebrez commented Oct 6, 2017

@ata2001 should I test the isorec use case?

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Oct 7, 2017

Member

@drebrez Please do if you have time, I could only test it on a bootimg device.

Member

ata2001 commented Oct 7, 2017

@drebrez Please do if you have time, I could only test it on a bootimg device.

@ata2001

This comment has been minimized.

Show comment
Hide comment
@ata2001

ata2001 Oct 14, 2017

Member

@drebrez Thank you for testing and reviewing, I believe I made the requested changes.

Member

ata2001 commented Oct 14, 2017

@drebrez Thank you for testing and reviewing, I believe I made the requested changes.

@drebrez

This comment has been minimized.

Show comment
Hide comment
@drebrez

drebrez Oct 14, 2017

Member

@ata2001 @ollieparanoid tested the isorec use case again and it works perfectly.

I was thinking that since it's just a small script and doesn't use much space, would be good to always install it, so add it as a dependency of postmarketos-base, what you think?

Member

drebrez commented Oct 14, 2017

@ata2001 @ollieparanoid tested the isorec use case again and it works perfectly.

I was thinking that since it's just a small script and doesn't use much space, would be good to always install it, so add it as a dependency of postmarketos-base, what you think?

@ollieparanoid

This comment has been minimized.

Show comment
Hide comment
@ollieparanoid

ollieparanoid Oct 14, 2017

Member

Thanks @ata2001 and @drebrez!

I was thinking that since it's just a small script and doesn't use much space, would be good to always install it, so add it as a dependency of postmarketos-base, what you think?

As discussed in the chat, I recommend against adding this to postmarketos-base, as long as it depends on util-linux (for findfs). Because when we have util-linux in the base installation, it will lead us to write code that does not work with just busybox installed.

Member

ollieparanoid commented Oct 14, 2017

Thanks @ata2001 and @drebrez!

I was thinking that since it's just a small script and doesn't use much space, would be good to always install it, so add it as a dependency of postmarketos-base, what you think?

As discussed in the chat, I recommend against adding this to postmarketos-base, as long as it depends on util-linux (for findfs). Because when we have util-linux in the base installation, it will lead us to write code that does not work with just busybox installed.

@ollieparanoid ollieparanoid merged commit 477f4c9 into master Oct 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ollieparanoid ollieparanoid deleted the feature/kernel-update branch Oct 14, 2017

@PureTryOut PureTryOut referenced this pull request Oct 18, 2017

Closed

Kernel updates #478

PureTryOut added a commit that referenced this pull request Feb 21, 2018

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