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.

Sign up for GitHub

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

Jump to bottom

Refactor Azure Pipelines config and tweak releases #244

Merged
merged 2 commits into from
Aug 6, 2019
Merged

Refactor Azure Pipelines config and tweak releases #244

merged 2 commits into from
Aug 6, 2019

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Aug 5, 2019 •
edited
Loading

  • Extract out doc/rustfmt jobs into their own separate builders. Helps
    avoiding having to skip tons of steps and can get failing results more
    quickly.

  • Extract out Rust installation logic to a dedicated template.

  • Separate out the build/test job matrices, where one matrix runs tests
    and another runs a full build

  • Refactor release directory structure to follow a convention where
    foo.tar.gz extracts to a folder called foo and follow unix-like
    conventions and copy over the license/readme files into the release
    tarballs.

@alexcrichton alexcrichton changed the title Change release archive directory structure Refactor Azure Pipelines config and tweak releases Aug 5, 2019
@alexcrichton alexcrichton force-pushed the tweak-azure branch 2 times, most recently from 10a8d35 to ff09f08 Compare August 5, 2019 19:40
@alexcrichton
Copy link
Member Author

Er sorry I botched the commit message and opened this PR with the wrong title, now the PR/description match the intended purpose!

@alexcrichton alexcrichton force-pushed the tweak-azure branch 5 times, most recently from 5d05220 to 9a7bd2d Compare August 5, 2019 19:56
@alexcrichton
Copy link
Member Author

Ok CI looks good here, although I'd like to test out the artifacts to make sure they've got the intended directory structure.

@tschneidereit
Copy link
Member

Thank you for doing these cleanups!

Ok CI looks good here, although I'd like to test out the artifacts to make sure they've got the intended directory structure.

I haven't yet looked at the PR in detail, but I did quickly look at the artifacts in the Pipelines artifacts explorer. I see two issues:

  1. at least for non-tag builds, the name doesn't include the tagName, in this case dev. E.g., the Windows archive has the name wasmtime--x86_64-windows.zip.
  2. there's a spurious additional folder in the archives: they contain the structure b/[archive basename]/[files], e.g. b/wasmtime--x86_64-windows/wasmtime.exe...

I'll look at the actual diff tomorrow.

tschneidereit
tschneidereit requested changes Aug 6, 2019
View reviewed changes
Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

This all looks great!

Apart from the issues mentioned below, I do have one concern: this eliminates all test coverage of release builds. While for the time being it doesn't seem extremely likely that we'd run into issues that only occur in release builds, if we did we'd find out much later. Also, it seems unfortunate not to test the bits that we're actually shipping. Hence, I'd prefer if we did include that test coverage still.

.azure-pipelines.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated Show resolved Hide resolved
.azure-pipelines.yml Outdated
condition: and(succeeded(), ne( variables['Agent.OS'], 'Windows_NT' ), eq( variables['build_type'], 'release' ))
set -e
mkdir -p $BUILD_BINARIESDIRECTORY/$BASENAME
if [ "$AGENT_OS" = "windows" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This is, of course, much better! :)

@alexcrichton alexcrichton force-pushed the tweak-azure branch 3 times, most recently from 7025fa0 to 6c29695 Compare August 6, 2019 14:41
@alexcrichton
Copy link
Member Author

Ok I think the tarballs are good to go now. I've pushed a final commit to remove a few more wip comments but it's the same config files as the last build which had successful artifacts that are all structured right

r? @tschneidereit

@kubkon
Copy link
Member

kubkon commented Aug 6, 2019

@alexcrichton @tschneidereit this is slightly off-topic however not all irrelevant, I've seen you guys use "cc" and "r?" in this repo and wasi-common. I was wondering, do you think it would be useful if we had bors set up for the two repos, or is an overkill at this stage?

@alexcrichton
Copy link
Member Author

I would personally consider it overkill in the sense that bors is best for when CI takes a long time, but the CI for both these repos is pretty speedy.

@kubkon
Copy link
Member

kubkon commented Aug 6, 2019

I would personally consider it overkill in the sense that bors is best for when CI takes a long time, but the CI for both these repos is pretty speedy.

Roger that :-)

@tschneidereit
Copy link
Member

@alexcrichton, see my comment about test coverage. Is there a reason why losing test coverage for release builds wouldn't be an issue?

@alexcrichton
Copy link
Member Author

Oh oops sorry totally missed that, agreed on that point and I'll add that back in.

@alexcrichton
Refactor Azure Pipelines config and tweak releases
24943c1 
* Extract out doc/rustfmt jobs into their own separate builders. Helps
  avoiding having to skip tons of steps and can get failing results more
  quickly.

* Extract out Rust installation logic to a dedicated template.

* Separate out the build/test job matrices, where one matrix runs tests
  and another runs a full build

* Refactor release directory structure to follow a convention where
  `foo.tar.gz` extracts to a folder called `foo` and follow unix-like
  conventions and copy over the license/readme files into the release
  tarballs.
@alexcrichton
Copy link
Member Author

Ok should be good to go!

tschneidereit
tschneidereit approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Looks great!

The one thing to change is the order of tests and builds for release.

.azure-pipelines.yml Show resolved Hide resolved
@tschneidereit tschneidereit merged commit 0616062 into bytecodealliance:master Aug 6, 2019
@alexcrichton alexcrichton deleted the tweak-azure branch August 6, 2019 16:33
grishasobol pushed a commit to grishasobol/wasmtime that referenced this pull request Nov 29, 2021
@Robbepop
Applies all clippy suggestions (bytecodealliance#244)
dd87384 
* Applies all clippy suggestions

In a few cases a clippy annotation was provided to ignore the clippy warning.

* apply rustfmt

* make wasmi compiler under no_std again

* use rustfmt +stable
mooori pushed a commit to mooori/wasmtime that referenced this pull request Mar 7, 2024
@aborg-dev
Merge pull request bytecodealliance#244 from near/start_to_main
207336a 
Use "main" instead of start function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tschneidereit tschneidereit tschneidereit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexcrichton @tschneidereit @kubkon