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

storage: no need in HFS+ on Apple Macs #4865

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

mikhailnov
Copy link
Contributor

Tests have shown that:

It is not clear what for the removed code existed. This patch fixes installation on x86 macs.
Did not test on ARM-based macbooks (I think they should also simply follow the UEFI specification and do not require removed hacks).

Co-authored with @temaps

@github-actions github-actions bot added the f39 label Jun 27, 2023
@jkonecny12
Copy link
Member

/packit build

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

We don't have Mac for testing so I would go with just smoke testing in this case.

@jkonecny12
Copy link
Member

So a few notes to this:

it seems that this code is here for a long time. It was originally named hfs+ and introduced here: 383a190 in RHEL-5. So there is low chance we will find more about it :D

About the PR, please remove all the occurrences of the class - see the tests.

@VladimirSlavik
Copy link
Contributor

Doesn't this effectively mean we stop supporting the intel macs as an installation target?

@mikhailnov
Copy link
Contributor Author

mikhailnov commented Jul 1, 2023

Doesn't this effectively mean we stop supporting the intel macs as an installation target?

It does not, because Intel Macs understand FAT16 and FAT32, and macOS there itself makes a FAT UEFI partition, but cannot create an HFS+ UEFI partition. This will allow to reuse the EFI with macos.

@mikhailnov
Copy link
Contributor Author

Both variants work:

  • reuse the FAT UEFI partition from macOS
  • create another FAT partition

@mikhailnov
Copy link
Contributor Author

We tested with macbooks from 2009, 2010 and probably from 2008 and a bit later ones. Is is possible to create a Fedora LiveCD with Anaconda with this patch so that I would test Fedora on them with this change? We tested ROSA.

@mikhailnov
Copy link
Contributor Author

Another user tested it (https://abf.io/platforms/rosa2021.1/products/316/product_build_lists/47297) on Macbook Air 13 from 2016 and reported that Anaconda works correctly there.

@VladimirSlavik VladimirSlavik self-assigned this Sep 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

This PR is stale because it has been open 60 days with no activity.
Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the stale label Nov 7, 2023
Copy link

github-actions bot commented Dec 7, 2023

This PR was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Dec 7, 2023
@poncovka
Copy link
Contributor

poncovka commented Dec 7, 2023

This was closed by accident. The team wants to merge it.

@pep8speaks
Copy link

pep8speaks commented Dec 7, 2023

Hello @mikhailnov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 34:100: E501 line too long (110 > 99 characters)

Comment last updated at 2023-12-08 14:25:22 UTC

@poncovka poncovka removed the stale label Dec 7, 2023
Artem Proskurnev and others added 2 commits December 8, 2023 14:59
Tests have shown that:
* HFS-specific code in grub-install utility contains silly mistakes (https://abf.io/import/grub2/blob/1145a32d77/0001-grub-install-fix-creating-mach_kernel-on-x86-mac.patch)
* even whith that patch, that code is broken and registers an incorrect bootloader entry via efibootmgr in UEFI
  (we in ROSA patch Anaconda to call grub2-install directly instead of calling efibootmgr inside Anaconda for unification)
* macOS installer does not allow to create an HFS+ partition for EFI and creates a FAT partition by default

It is not clear what for the removed code existed.
This patch fixes installation on x86 macs.
Did not test on ARM-based macbooks (I think they should also simply follow the UEFI specification and do not require removed hacks).

Co-authored-by: Mikhail Novosyolov <[email protected]>
Clean up after the 708a8f9 commit.
@poncovka
Copy link
Contributor

poncovka commented Dec 8, 2023

/kickstart-test --testtype smoke

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@poncovka poncovka merged commit 60af090 into rhinstaller:master Dec 14, 2023
15 of 16 checks passed
@mikhailnov
Copy link
Contributor Author

Great, thanks! 😊

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

Successfully merging this pull request may close these issues.

5 participants