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

Support for Windows Version and Edition Information #45

Merged
merged 9 commits into from
Dec 12, 2017
Merged

Support for Windows Version and Edition Information #45

merged 9 commits into from
Dec 12, 2017

Conversation

mattmccarty
Copy link
Contributor

I have tested this on Windows 10 and it works great, but that's the only version of Windows I have access to. Need help with testing this on various versions of Windows.

>cargo test -- --nocapture
   Compiling os_info v0.4.
    Finished dev [unoptimized + debuginfo] target(s) in 1.39 secs
     Running target\debug\deps\os_info-499680dd31699698.exe

running 1 test
**Windows Version**: _10.0.15063_
**Windows Edition**: _Windows 10_
test imp::version::tests::parses_version ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

   Doc-tests os_info

running 1 test
test src\lib.rs - get (line 48) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

@mattmccarty mattmccarty mentioned this pull request Dec 9, 2017
Copy link
Owner

@stanislav-tkach stanislav-tkach left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request!

As you can see, the build is unsuccessful because of rust-fmt. I don't like its formatting either, but I still prefer to have consistency.

Please apply formatting and fix the issue with osvi, then I will be happy to merge your request.

None => Self::osvi()
};

let osvi = info.osvi.unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

What if RtlGetVersion fails? Then osvi() function will return Win32Version with None in the osvi field, right? After that Win32Version::osvi is going to be called unsuccessfully again which gives panic.

Am I missing something? If not - it would be better to handle it somehow.

@mattmccarty
Copy link
Contributor Author

mattmccarty commented Dec 10, 2017

Hi! I'm new to rust, so I appreciate your feedback. I'll make the changes that you requested and commit.

Do you have any idea how to fix the compile issues for the "x86_64-pc-windows-gnu" target? I was able to reproduce the issue in cygwin, but cannot find a solution. I typically use linux, so maybe I'm missing something. I was about to open a thread on stackoverflow when I saw your reply.

I tried the solution found here, but that didn't seem to work either.

@mattmccarty
Copy link
Contributor Author

Obviously my local version of rustfmt isn't working the same as the version in the CI. I will fix the rest of these formatting errors here shortly.

@stanislav-tkach
Copy link
Owner

I tried the solution found here, but that didn't seem to work either.

Unfortunately, I don't know how to fix it either. I'm going to do a little investigation, but I welcome any help here. 😄
As workaround blank implementation for the GNU targets can be added, but I'm not sure if there is a way to distinguish between GNU and MSVC targets.

I'm using 0.9.0 version of rustfmt (https://github.com/DarkEld3r/os_info/blob/master/.travis.yml#L16) by the way.

@mattmccarty
Copy link
Contributor Author

mattmccarty commented Dec 11, 2017

Cool. I opened the question on stackoverflow. Maybe someone can chip in,

@stanislav-tkach
Copy link
Owner

@mattmccarty I have found the following here:

target_env = ".." - Further disambiguates the target platform with information about the ABI/libc. Presently this value is either "gnu", "msvc", "musl", or the empty string. For historical reasons this value has only been defined as non-empty when needed for disambiguation. Thus on many GNU platforms this value will be empty. This value is closely related to the fourth element of the platform target triple, though it is not identical. For example, embedded ABIs such as gnueabihf will simply define target_env as "gnu".

@mattmccarty
Copy link
Contributor Author

I tried using target env, but the issue persisted. I took a look at how the developers of the winapi crate did it. It appears that we need to include the libntdll.a file in our repo and tell gcc to include the directory at build time. This happens by creating a custom build.rs file.

So the build works on x86_64 targets, but fails on i686. Need to figure out how to fix that next. It seems like the DLL library isn't being included.

@mattmccarty
Copy link
Contributor Author

Nevermind, it's probably because I'm not on an i686 machine.

@stanislav-tkach stanislav-tkach merged commit 4d44974 into stanislav-tkach:master Dec 12, 2017
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.

3 participants