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

Simplify ARCH detection and rework reading /proc/cpuinfo into CPUINFO #9981

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zopolis4
Copy link
Collaborator

Now that we can set ARCH via an environment variable, there's no need to have all the awkward workarounds for containers, as they can simply set the desired architecture.

Run the following to get this pull request's changes locally for testing.

CREW_REPO=https://github.com/Zopolis4/chromebrew.git CREW_BRANCH=back crew update

@satmandu
Copy link
Member

The detection code is useful, I think, since we use minor corrections to what the containers give us to emulate what shows up on actual hardware.

It's not just a matter of forcing an ARCH value...

@Zopolis4
Copy link
Collaborator Author

Isn't it? To me this just seems like leftover code from the days when we couldn't set ARCH via an environment variable, so this can all be replaced by just setting ARCH as armv7l in a container that would otherwise report armv8l.

@satmandu
Copy link
Member

The code may be useful, and I'm not comfortable getting rid of it while we are still having architecture specific issues that might be more granular than we assume...

@Zopolis4
Copy link
Collaborator Author

Do we have any of those issues outstanding? I was under the impression that most of this code wasn't getting used anyways...

@satmandu
Copy link
Member

Do we have any of those issues outstanding?

We still don't have aarch64 working!

I was under the impression that most of this code wasn't getting used anyways...

I think it is premature to assume that when we still have architectures not working properly. And aarch64 user space definitely fits that.

That is I think reason enough to keep this plumbing in for now...

@Zopolis4
Copy link
Collaborator Author

This code is only for containers, though. Thats not particularly related to the aarch64 situation.

@satmandu
Copy link
Member

99% of the testing (not just for new architectures) happens in containers... And that goes for aarch64 too.

I literally don't have an aarch64 ChromeOS device ... Everything I do is in containers.

@Zopolis4
Copy link
Collaborator Author

Just to be clear-- all the code around CPU_SUPPORTED_ARCH is unused. Same with QEMU_EMULATED.

Neither of those variables are used.

The only change I'm making is that armv8l (only encountered in certain container setups) is no longer automatically converted to armv7l, and now the ARCH environment variable has to be used.

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.

None yet

2 participants