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

Add AppVeyor CI configuration #46

Merged
merged 1 commit into from
Jul 13, 2019
Merged

Add AppVeyor CI configuration #46

merged 1 commit into from
Jul 13, 2019

Conversation

rivy
Copy link
Contributor

@rivy rivy commented Mar 13, 2019

  • includes testing to minimum working version (1.17.0)

* includes testing to minimum working version (1.17.0)
@rivy
Copy link
Contributor Author

rivy commented Mar 13, 2019

@ogham , for this happen automatically, you'll need to set up an AppVeyor project which observes this repository (similar to Travis CI; see https://www.appveyor.com/docs).

A current completed test run is at https://ci.appveyor.com/project/rivy/rust-ansi-term/builds/23029890.

@retep998
Copy link

This appveyor.yml is overly complicated given this crate is pure Rust. Most of that environment setup stuff is totally extraneous and can be stripped out. For a simpler example see winapi's own appveyor.yml.

@rivy
Copy link
Contributor Author

rivy commented Mar 13, 2019

Well, @retep998 , I generally disagree.

Although it's not as simple as the winapi example, it was created for a broader use case and modeled after needs in "coreutils". Ultimately, this version is generally useful, documented, logs what it's doing and the environment that it is using, and has some workarounds for git/coverage and the gnu library problems mentioned in rust-lang/rust#47048 and rust-lang/rust#53454 (modeled after your suggestion). Yes, the gnu library workaround code is verbose, but, it's not certain when a dependent crate might bring in code that causes the library conflict.

Overall, I'm just offering a working starting point for the author to help advance the windows version of this story. It could obviously be tailored, and I'm open to suggested changes... or someone else could throw up a completely different working solution.


# "msvc" ABI setup
- if /i "%ABI%" == "msvc" if /i "%ARCH%" == "i686" call "%VS140COMNTOOLS%\..\..\VC\vcvarsall.bat"
- if /i "%ABI%" == "msvc" if /i "%ARCH%" == "x86_64" call "C:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x64

Choose a reason for hiding this comment

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

Why such an old Windows SDK?

- if /i "%ABI%"=="gnu" ( where gcc && gcc --version )

# "msvc" ABI setup
- if /i "%ABI%" == "msvc" if /i "%ARCH%" == "i686" call "%VS140COMNTOOLS%\..\..\VC\vcvarsall.bat"

Choose a reason for hiding this comment

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

If you ever wanted to support cross compilation from one arch to another, say to build for arm or aarch64, you would not be able to use the vcvars bat files as it would affect both build scripts compiled for the host architecture, and binaries compiled for the target architecture.

@retep998
Copy link

For crates that have C dependencies, yes getting MinGW setup correctly is important, and so your appveyor.yml is useful generally. However this crate only has winapi as a dependency, and so doesn't need any of the MinGW setup. Ultimately though, it's up to the crate author to decide what they want in terms of appveyor.

## * workaround for rust-lang/rust#47048 / rust-lang/rust#53454 ## !maint: remove when resolved
# ** ref: <https://github.com/rust-lang/rust/issues/47048>, <https://github.com/rust-lang/rust/issues/53454>
# ** egs: <https://github.com/pkgw/tectonic/commit/29686db533d8732d7d97fc94270ed33b77f29295>, <https://github.com/rukai/PF_Sandbox/blob/e842613cf9ff102dfb3fbd87381319e6e6dfe3ae/appveyor.yml>
- if /i "%ABI%"=="gnu" rustup install %CHANNEL%-%ARCH%-pc-windows-msvc

Choose a reason for hiding this comment

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

It seems wasteful to have installed a toolchain with rustup up above, only to then install a different toolchain entirely here.

@ogham ogham merged commit 60dac8c into ogham:master Jul 13, 2019
@ogham
Copy link
Owner

ogham commented Jul 13, 2019

Hey,

I signed up for AppVeyor. I'm still slowly learning how the Windows ecosystem works — as a crate maintainer I should really have more knowledge of it than I do right now. I'm going to merge this PR because I still want to have CI for Windows, and eventually I'd like to have learned the best way to do it myself.

Now to see if the build works...

Thanks 👍

@rivy
Copy link
Contributor Author

rivy commented Jul 20, 2019

I fixed a couple of things for this in PR #52, and I'm happy to help you set up a working automatic build/test system that includes windows.

sholderbach referenced this pull request in nushell/nu-ansi-term Mar 14, 2022
Add AppVeyor CI configuration
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