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

Initial arm64 support #214

Merged
merged 1 commit into from Oct 25, 2023
Merged

Initial arm64 support #214

merged 1 commit into from Oct 25, 2023

Conversation

mika
Copy link
Member

@mika mika commented Oct 13, 2023

Based on #191 by @GavinPacini - rebased on top of current master to address merge conflict, squashed commits into one single commit, minor styling corrections.

Closes: #169

@mika mika mentioned this pull request Oct 13, 2023
@adrelanos
Copy link
Contributor

Unfortunately, a new merge conflict.

@mika
Copy link
Member Author

mika commented Oct 16, 2023

Unfortunately, a new merge conflict.

But easy to handle, done

@adrelanos
Copy link
Contributor

#191 (comment)

grml-debootstrap Outdated
fi
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

<<<<<<< HEAD is probably a mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhm yeah, sorry, pushed the correct version now

grml-debootstrap Outdated
@@ -1626,26 +1679,27 @@ if [[ -z "${GRUB}" ]] || ! dd if="${GRUB}" bs=512 count=1 2>/dev/null | cat -v |
fi
chroot "$MNTPOINT" grub-install --target=x86_64-efi --efi-directory=/boot/efi --uefi-secure-boot --removable "/dev/$LOOP_DISK"
chroot "$MNTPOINT" grub-install --target=i386-pc "/dev/$LOOP_DISK"
=======
Copy link
Contributor

Choose a reason for hiding this comment

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

This too.

grml-debootstrap Outdated

dd if="${MNTPOINT}/tmp/core.img" of="${ORIG_TARGET}" conv=notrunc seek=1
rm -f "${MNTPOINT}/tmp/core.img"
>>>>>>> ecddb6f (Initial arm64 support)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

@mika
Copy link
Member Author

mika commented Oct 16, 2023

I think grub_install() could be simplified now with f4d3906 being present?

@adrelanos
Copy link
Contributor

adrelanos commented Oct 19, 2023

git checkout 670035bdde9ac232b3869eea4a66f5412d1d1132
sudo make install

670035b build + boot success.

@mika
Copy link
Member Author

mika commented Oct 19, 2023

git checkout 670035bdde9ac232b3869eea4a66f5412d1d1132
sudo make install

670035b build + boot success.

Great, thanks for testing, @adrelanos! :) Is @GavinPacini somehow connected/related to you/your work? I'm wondering whether to accept this PR which I did with some commit squashing and style adjustments but under Gavin's name is a) fine for him, and b) whether we could further improve current state? (Also making sure that we didn't break non-arm64 would be good :))

@adrelanos
Copy link
Contributor

I cannot speak for @GavinPacini.

Is @GavinPacini somehow connected/related to you/your work?

That shouldn't matter. I hope it doesn't matter. I didn't think any transparency is required here. But for full transparency...

Gavin has been active here:
https://forums.whonix.org/t/whonix-on-mac-m1-arm-user-support-still-unsupported-at-time-of-writing/11310

At some point I said something like "the first step to make Whonix on ARM64 work would be contributing #169 to upstream grml-debootstrap". Gavin contributed towards that.

The pull request was made from grml-debootstrap (upstream) perspective. So upstream doesn't need to spend time on any unrelated history.

I'm wondering whether to accept this PR which I did with some commit squashing and style adjustments but under Gavin's name is a) fine for him,

So the easy, proper attribution so we don't have to wait for Gavin or anyone's input would be... Merge Gavin's pull request "somehow" if that's still possible. Quick and dirty. Then in the next commit "revert" to your 670035b.

Otherwise attribution in the commit message should be sufficient too?

When I look at https://github.com/grml/grml-debootstrap/pull/214/commits it reads:

@GavinPacini authored and @mika committed.

So it seems that would already as of now been properly recorded in the git history?

b) whether we could further improve current state?

I hope that could be done in form of new issues posted and new pull requests?

(Also making sure that we didn't break non-arm64 would be good :))

I can test booworm amd64 now.

@mika
Copy link
Member Author

mika commented Oct 19, 2023

I cannot speak for @GavinPacini.

Is @GavinPacini somehow connected/related to you/your work?

That shouldn't matter. I hope it doesn't matter. I didn't think any transparency is required here. But for full transparency...

Gavin has been active here: https://forums.whonix.org/t/whonix-on-mac-m1-arm-user-support-still-unsupported-at-time-of-writing/11310

At some point I said something like "the first step to make Whonix on ARM64 work would be contributing #169 to upstream grml-debootstrap". Gavin contributed towards that.

Ah no worries, I just wanted to know whether you could reach out to Gavin since he didn't respond in the last few days. :)

The pull request was made from grml-debootstrap (upstream) perspective. So upstream doesn't need to spend time on any unrelated history.

I'm wondering whether to accept this PR which I did with some commit squashing and style adjustments but under Gavin's name is a) fine for him,

So the easy, proper attribution so we don't have to wait for Gavin or anyone's input would be... Merge Gavin's pull request "somehow" if that's still possible. Quick and dirty. Then in the next commit "revert" to your 670035b.

Otherwise attribution in the commit message should be sufficient too?

When I look at https://github.com/grml/grml-debootstrap/pull/214/commits it reads:

@GavinPacini authored and @mika committed.

So it seems that would already as of now been properly recorded in the git history?

Absolutely, the author would be Gavin, and I'm the committer only, would be fine for me

b) whether we could further improve current state?

I hope that could be done in form of new issues posted and new pull requests?

Also fine for me :)

(Also making sure that we didn't break non-arm64 would be good :))

I can test booworm amd64 now.

I'd certainly appreciate that, once we know we're good I'd merge this PR and we can continue work from there then :)

thx!

@adrelanos
Copy link
Contributor

Bookworm amd64 build + boot success.

qemu-system-x86_64 \
    -m 1024 \
    -drive format=raw,file=/home/user/grml-debootstraptestbin/test.img

Ah no worries, I just wanted to know whether you could reach out to Gavin since he didn't respond in the last few days. :)

Unfortunately, I don't have any other contact.

@mika
Copy link
Member Author

mika commented Oct 19, 2023

Bookworm amd64 build + boot success.

qemu-system-x86_64 \
    -m 1024 \
    -drive format=raw,file=/home/user/grml-debootstraptestbin/test.img

Ok great, thanks for testing!

Ah no worries, I just wanted to know whether you could reach out to Gavin since he didn't respond in the last few days. :)

Unfortunately, I don't have any other contact.

No worries, then I suggest we wait until next week, and if we don't have any objections until then from @GavinPacini I'd say we merge this PR. :)

@mika mika merged commit ad99b61 into master Oct 25, 2023
@mika
Copy link
Member Author

mika commented Oct 25, 2023

Ok, merged now! :) Thanks everyone!

@GavinPacini
Copy link
Contributor

Great to see that this was completed and merged. Thanks @adrelanos and @mika for getting this over the line!

I have not been as active on open-source contributions lately, apologies, but I do appreciate your support in finishing this. The growing arm64 community will be very happy I'm sure.

@mika
Copy link
Member Author

mika commented Jan 26, 2024

@GavinPacini wonderful, thanks for reporting back! :)

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

Successfully merging this pull request may close these issues.

ARM support
3 participants