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

Install VcPkg/Pkg-Config depending on target env #56

Merged
merged 1 commit into from
May 4, 2021

Conversation

filips123
Copy link
Contributor

Although #39 was merged, Cargo.toml still required VcPkg on Windows, regardless of used toolchain. This meant that when someone tried to build compress-tools on windows-gnu, build script failed because it wasn't able to find pkg-config module.

This PR updates Cargo.toml to require either VcPkg or Pkg-Config depending on target env, just like build.rs since #39. It also updates docs and readme to indicate this.

@coveralls
Copy link

coveralls commented May 4, 2021

Pull Request Test Coverage Report for Build 810866131

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 85.302%

Totals Coverage Status
Change from base Build 797784751: 0.5%
Covered Lines: 325
Relevant Lines: 381

💛 - Coveralls

@otavio
Copy link
Member

otavio commented May 4, 2021

@filips123 the change is good. Could you add a note on the CHANGES.md file and squash all commits?

@otavio
Copy link
Member

otavio commented May 4, 2021

and ... as you're involved in Windows GNU support, maybe you could prepare another PR adding a CI check for it?

@filips123
Copy link
Contributor Author

Could you add a note on the CHANGES.md file and squash all commits?

Sure.

maybe you could prepare another PR adding a CI check for it?

I will probably try next week.

@otavio otavio merged commit 6eaf5af into OSSystems:master May 4, 2021
@filips123
Copy link
Contributor Author

filips123 commented May 10, 2021

I added CI check for Windows GNU. Most tests are passing, but there is a segmentation fault (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION) while running integration test: https://github.com/filips123/compress-tools-rs/runs/2544416808#step:10:129

I think it's caused by uncompress_to_dir_with_utf8_pathname. And it happens at CStr::from_ptr(ffi::archive_entry_pathname(entry)) in https://github.com/OSSystems/compress-tools-rs/blob/master/src/lib.rs#L217 (specifically, when calling CStr::from_ptr).

@otavio
Copy link
Member

otavio commented May 10, 2021

It is likely to be the case. For it to run a proper locale must be in use. Try setting LC_ALL=en_US.UTF-8 as environment.

@filips123
Copy link
Contributor Author

Unfortunately that didn't help.

@otavio
Copy link
Member

otavio commented May 10, 2021

@filips123 this is indeed quite hard to figure out without a proper environment to try it. The code involved in this is:

#[cfg(windows)]
mod inner {
extern "C" {
fn _configthreadlocale(arg1: std::os::raw::c_int) -> std::os::raw::c_int;
}
const _ENABLE_PER_THREAD_LOCALE: std::os::raw::c_int = 1;
pub(crate) struct UTF8LocaleGuard {
save: Option<std::ffi::CString>,
save_thread_config: ::std::os::raw::c_int,
}
impl UTF8LocaleGuard {
pub(crate) fn new() -> Self {
let locale = b".UTF-8\0";
let (save, save_thread_config) = {
let old_locale = unsafe { libc::setlocale(libc::LC_CTYPE, std::ptr::null()) };
(
if old_locale.is_null() {
None
} else {
Some(unsafe { std::ffi::CStr::from_ptr(old_locale) }.to_owned())
},
unsafe { _configthreadlocale(0) },
)
};
unsafe {
_configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
libc::setlocale(
libc::LC_CTYPE,
std::ffi::CStr::from_bytes_with_nul_unchecked(locale).as_ptr(),
)
};
Self {
save,
save_thread_config,
}
}
}

Please see if you can reproduce it on your Windows machine.

@filips123
Copy link
Contributor Author

Yes, I can reproduce it.

@otavio
Copy link
Member

otavio commented May 10, 2021

So we'll need to figure out if the MSVC and GNU Windows support use the same schema for locale. I have no idea.

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

Successfully merging this pull request may close these issues.

3 participants