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

First draft of Azure Pipelines CI config #191

Merged
merged 9 commits into from
Jul 22, 2019
Merged

First draft of Azure Pipelines CI config #191

merged 9 commits into from
Jul 22, 2019

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jul 16, 2019

You'll have to follow the instructions over at https://github.com/crate-ci/azure-pipelines to get it fully set up. Might also run into some weirdness with having to change the "default" branch once merged. But 🎉

Oh, yeah, and need to also update Cargo.toml, README badge, etc.

@jonhoo jonhoo requested a review from hawkw July 16, 2019 19:08
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This looks good to me at a glance, and I'd be happy to merge it once the various badges etc have also been updated. I had some questions about what the CI config is doing

azure-pipelines.yml Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
azure-pipelines.yml Show resolved Hide resolved
@jonhoo jonhoo requested a review from hawkw July 17, 2019 21:09
@hawkw
Copy link
Member

hawkw commented Jul 17, 2019

@jonhoo should I do the steps you linked above to enable Azure Pipelines before or after the configs are merged to master?

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this LGTM & I'll be very happy to hit the merge button once i know what order to do the rest of the setup in :)

@hawkw
Copy link
Member

hawkw commented Jul 17, 2019

@jonhoo oh, can we get github.io RustDoc publishing on commits to master, eventually? It's fine if that lands in a follow-up branch...

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 17, 2019

@hawkw you do the steps first, and when it asks to "select branch" you just select this branch instead of master. we can then update the "default branch" after this PR has been merged.

RustDoc publishing should also totally be doable, but isn't something I have any experience with. I know that you can enable access to git commands as outlined here, and then it should just be a matter of (conditionally on the branch being master) running the appropriate commands to update the gh-pages branch. If you do write that I think it'd be an excellent addition to crate-ci too (cc @epage)! There's also this neat Crate a GitHub Release task that may come in handy in the future.

@hawkw
Copy link
Member

hawkw commented Jul 17, 2019

@hawkw you do the steps first, and when it asks to "select branch" you just select this branch instead of master. we can then update the "default branch" after this PR has been merged.

Okay, looks like only @carllerche has the permissions to edit what repos in the tokio-rs org Azure can access; according to github I've "requested" that he approve Azure Pipelines access for this repo & hopefully he'll turn it on when he gets the chance.

RustDoc publishing should also totally be doable, but isn't something I have any experience with. I know that you can enable access to git commands as outlined here, and then it should just be a matter of (conditionally on the branch being master) running the appropriate commands to update the gh-pages branch. If you do write that I think it'd be an excellent addition to crate-ci too (cc @epage)! There's also this neat Crate a GitHub Release task that may come in handy in the future.

Okay, let's open a follow-up issue to track adding this? IIRC tokio-rs/tokio already has doc publishing set up, so we can perhaps borrow that...

@hawkw
Copy link
Member

hawkw commented Jul 19, 2019

@jonhoo it looks like there's an error in the Azure Pipelines config: https://dev.azure.com/tracing/tracing/_build/results?buildId=1&view=results

azure-pipelines.yml Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Jul 19, 2019

Looks like this is blocked on either making the readme doctest thing work on stable Rust, or removing it? (cc @yaahallo)

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 19, 2019

Or alternatively not running cargo check --all-features on CI, but that seems unfortunate.

@hawkw
Copy link
Member

hawkw commented Jul 19, 2019

@jonhoo I spoke to @yaahallo offline & she says that if we disable the readme doctest for now and open an issue to get it working on stable, she'll look into it later. If you wouldn't mind disabling it on this branch, I think we can move forwards with this PR?

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 21, 2019

Done!

@hawkw
Copy link
Member

hawkw commented Jul 21, 2019

@jonhoo can we also actually disable the readme doctest in tracing? i.e. comment out or remove these lines:

#![cfg_attr(feature = "doctest-readme", feature(external_doc))]

tracing/tracing/src/lib.rs

Lines 402 to 403 in 9d9ba34

#[cfg(feature = "doctest-readme")]
mod readme_doctest;

doctest-readme = []

so that the cargo check --all-features step actually passes?

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 21, 2019

Done!

@hawkw
Copy link
Member

hawkw commented Jul 22, 2019

Ah, it looks like we still can't run cargo check --all-features on the Rust 1.34.0 build, as it will pull in std::future, which is feature-flagged in the tracing-futures crate. Can we skip that stage on the Rust 1.34 job in particular?

@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 22, 2019

Ah, that will require crate-ci/azure-pipelines#42 (comment).

@hawkw
Copy link
Member

hawkw commented Jul 22, 2019

@jonhoo if we can just disable the cargo check stage entirely for now, and re-enable it in a followup PR when crate-ci supports making it conditional, that's fine with me as well.

We need support for _not_ running with `--all-features` due to the
nightly requirement for async/await which is enabled by a feature in
`tracing`. This will eventually land with
crate-ci/azure-pipelines#42.
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 22, 2019

@hawkw done!

@hawkw
Copy link
Member

hawkw commented Jul 22, 2019

@jonhoo great, looks like this is now working properly! I'm going to merge this PR now, and we can open new issues for the follow-up work --- would you like to open them, or shall I?

@hawkw hawkw merged commit b30821e into master Jul 22, 2019
@jonhoo
Copy link
Contributor Author

jonhoo commented Jul 22, 2019

I think you have a better idea of how important the different things are, and who to tag to fix them :)

@jonhoo jonhoo deleted the jonhoo/azure branch July 22, 2019 19:06
@jonhoo jonhoo mentioned this pull request Jul 22, 2019
hawkw pushed a commit that referenced this pull request Jul 22, 2019
Brings back `cargo check` as discussed here: 
#191 (comment) 
now that crate-ci/azure-pipelines#42 has landed.
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