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

Implement CpuArch #1126

Closed
wants to merge 53 commits into from
Closed

Implement CpuArch #1126

wants to merge 53 commits into from

Conversation

njouanin
Copy link
Contributor

@njouanin njouanin commented Nov 2, 2023

Fixes #1118.

May need some test on Linux, BSD and Windows (tested on MacOS)

src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
tests/cpu.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez changed the title Implement #1118 Implement CpuArch Nov 3, 2023
@GuillaumeGomez
Copy link
Owner

A lot of changes to do here to prevent keeping the String around and calling .clone() on CpuArch. Also, please make the arch method return an Option (and store it into an Option as well).

To make the conversion better, might be better to implement TryFrom<str> so we can return None in case this is an unknown arch.

Otherwise, thanks for starting this work!

Oh also, please squash your commits to not have the merge commits in the middle (you can check how to do it here).

@njouanin
Copy link
Contributor Author

njouanin commented Nov 3, 2023

Thanks for your feedback, I’m not a Rust expert yet 😅.
I’ll fix and improve this code.

@njouanin
Copy link
Contributor Author

njouanin commented Nov 3, 2023

@GuillaumeGomez : do you know how to fix this CI error (rust 1.65 on darwin) ?

error[E0658]: use of unstable library feature 'cstr_from_bytes_until_nul'
Error:    --> src/unix/apple/cpu.rs:398:9
    |
398 |         CStr::from_bytes_until_nul(&arch_str)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #95027 <https://github.com/rust-lang/rust/issues/95027> for more information

src/common.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

Please squash your commits into one (you can check how to do it here).

@njouanin
Copy link
Contributor Author

njouanin commented Nov 6, 2023

Please squash your commits into one (you can check how to do it here).

I'm afraid this is beyond my Git knowledge. Following your git tips I get errors and conflicts, may be due to pull requests I've done during this development:

Auto-merging src/windows/disk.rs
CONFLICT (content): Merge conflict in src/windows/disk.rs
error: could not apply b19e71b... Fix comment typo
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".

Can you handle squash and merge ?

@GuillaumeGomez
Copy link
Owner

If you have conflicts locally, github won't do any better. Best way: create a branch from my master and put all your changes into it and merge then force push here.

@njouanin njouanin closed this Nov 11, 2023
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.

Add CPU architecture info
5 participants