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

Statically link C-runtime for MSVC Windows #221

Merged
merged 4 commits into from
Jan 23, 2022
Merged

Statically link C-runtime for MSVC Windows #221

merged 4 commits into from
Jan 23, 2022

Conversation

ducaale
Copy link
Owner

@ducaale ducaale commented Jan 16, 2022

On windows, xh requires VS C++ Runtime Redistributable to be installed or will fail with The code execution cannot proceed because VCRUNTIME140.dll was not found. This PR changes that by statically linking the C-runtime.

Also, see BurntSushi/ripgrep#1613

Partially resolves #219

@blyxxyz
Copy link
Collaborator

blyxxyz commented Jan 16, 2022

Hm, is this really something we want to specify for the whole project? Maybe it should be limited to the release action.

@ducaale ducaale marked this pull request as draft January 16, 2022 21:53
@ducaale
Copy link
Owner Author

ducaale commented Jan 16, 2022

The other option would be to use the RUSTFLAGS environment variable. I will see how I can do that in .github/workflows/release.yaml.

@@ -77,6 +77,8 @@ jobs:
use-cross: ${{ matrix.job.use-cross }}
command: build
args: --release --target ${{ matrix.job.target }} ${{ matrix.job.flags }}
env:
RUSTFLAGS: ${{ matrix.job.os == 'windows-latest' && '-C target-feature=+crt-static' || '' }}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Should I set RUSTFLAGS in other places as well?

@ducaale ducaale marked this pull request as ready for review January 17, 2022 21:11
@blyxxyz
Copy link
Collaborator

blyxxyz commented Jan 17, 2022

Can you run the release action somewhere for a binary we can test with?

@ducaale
Copy link
Owner Author

ducaale commented Jan 17, 2022

Sure, I have created https://github.com/ducaale/xh-temp-111 for testing purposes. I'll grant you push access in a moment.

@blyxxyz blyxxyz self-requested a review January 21, 2022 23:44

- name: Set RUSTFLAGS env variable
if: matrix.job.rustflags
shell: bash
Copy link
Owner Author

Choose a reason for hiding this comment

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

Unless I set shell to bash, RUSTFLAGS will not get passed to cargo in the next step.

@blyxxyz
Copy link
Collaborator

blyxxyz commented Jan 23, 2022

Should we perhaps also move LTO from Cargo.toml to the release CI? An incremental rebuild takes 10s without it and 1m30s with, that's annoying when benchmarking.

@ducaale
Copy link
Owner Author

ducaale commented Jan 23, 2022

I will need to add Cross.toml file so cross can be configured to pass RUSTFLAGS to the underlying build environment.

Edit: I am currently experimenting with CARGO_PROFILE_RELEASE because enabling lto via RUSTFLAGS errors

$ RUSTFLAGS="-C lto" cargo build --release
   Compiling autocfg v1.0.1
   Compiling libc v0.2.101
   Compiling version_check v0.9.3
   Compiling proc-macro2 v1.0.29
error: options `-C embed-bitcode=no` and `-C lto` are incompatible

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Looks good! Unlike v0.14.1 it just works on a clean Windows installation.

@ducaale ducaale merged commit 5245a9d into develop Jan 23, 2022
@ducaale ducaale deleted the crt-static branch January 23, 2022 14:11
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.

2 participants