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

relauto: init pr builds #495

Merged
merged 26 commits into from
Nov 19, 2019
Merged

relauto: init pr builds #495

merged 26 commits into from
Nov 19, 2019

Conversation

jahkeup
Copy link
Member

@jahkeup jahkeup commented Nov 8, 2019

Issue #, if available:

#492

Description of changes:

This implements a basic build-on-pr buildspec for use in CodeBuild.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tools/infra/buildspec/thar-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/buildspec/thar-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/env/bin/absolutify-path Outdated Show resolved Hide resolved
tools/infra/env/bin/absolutify-path Outdated Show resolved Hide resolved
Comment on lines 15 to 16
- . "${INFRA_DIR}/env/lib/environment-setup"
- . install-rust-builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Why implement these as scripts that run when sourced vs. just running the scripts?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of these, we need the environment that's a result of running them.

  • environment-setup sets up the builder environment and modifies/exports variables like $PATH
  • install-rust-builder sets up the builder and adds rust + cargo to the $PATH

I'll also take this opportunity to point out that the shell used by CodeBuild is /bin/sh which is why ..

tools/infra/env/bin/install-rust-builder Outdated Show resolved Hide resolved
tools/infra/env/bin/write-build-manifest Outdated Show resolved Hide resolved
tools/infra/env/bin/write-build-meta Outdated Show resolved Hide resolved
tools/infra/env/bin/write-build-meta Outdated Show resolved Hide resolved
@jahkeup
Copy link
Member Author

jahkeup commented Nov 12, 2019

We'll want to drop the buildkit bits when #506 lands.

@jahkeup
Copy link
Member Author

jahkeup commented Nov 12, 2019

Rebased and updated to address some feedback from folks.

tools/infra/stacks/host-containers-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/stacks/host-containers-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/stacks/infra-pr-build.yml Show resolved Hide resolved
tools/infra/stacks/thar-pr-build.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🍔

Let's get this in and then iterate!

@jahkeup jahkeup changed the title relauto: init thar-pr-build relauto: init pr builds Nov 13, 2019
@jahkeup jahkeup marked this pull request as ready for review November 13, 2019 20:09
configure_color

# Use rustup to install rust toolchain
if ! curl --proto '=https' --tlsv1.2 -sS "https://sh.rustup.rs" | bash -s -- -y --no-modify-path --default-toolchain stable; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be pinning to a Rust version rather than taking the latest release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Answer: yes, we should probably be.

Longer answer:

We probably /should/ be pinning to a known version of rust, however, the moving parts affected by the build of rust used are: buildsys, cargo-make, and cargo-deny. All the other components build and run with rust are within the scope of the rust.spec version provided at build time in the build containers.

In the short term, tracking the stable version does give us some amount of certainty about the rust toolchain itself that's used; I think that's this is "good enough for the moment" because we don't have methods or assertions to "qualify" these components when moving to a different version of the rust toolchain.

As far as I know we're already tracking some version of the rust toolchain on each developers' machines - we could stand to improve some in the way of being able to truly vet a bump and tie our dependencies to a version of rust if that's the need.

Copy link
Member Author

Choose a reason for hiding this comment

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

fwiw: we're now locked to specific dependencies' versions:

  • cargo-make: 0.23.0
  • cargo-deny: 0.2.6

tools/infra/env/bin/start-buildkitd-daemon Outdated Show resolved Hide resolved
tools/infra/stacks/host-containers-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/stacks/host-containers-pr-build.yml Outdated Show resolved Hide resolved
tools/infra/stacks/host-containers-pr-build.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

One nit. Let's get this merged and iterate!

🍦

tools/infra/env/bin/setup-rust-builder Outdated Show resolved Hide resolved
This includes:

- Minor reordering and YAML tidying
- Use of shorthand syntax for !IntrinsicFns
- Use of an explicit LogGroup
- Introduction of Parameters for inputs to build resources
This prevents misconfiguration where either the tool has run before or
in another unassuming environment.
PR #506 switches us away from needing a daemon running.

Signed-off-by: Jacob Vallejo <[email protected]>
@jahkeup
Copy link
Member Author

jahkeup commented Nov 15, 2019

Force pushed to catch up to develop and use Docker 👍

@jahkeup
Copy link
Member Author

jahkeup commented Nov 18, 2019

Current build failure is a true positive, I think!

@iliana does this look like a true failing case to you?

https://gist.github.com/jahkeup/8b9c39976408d197e4699f39ae9e4855

@iliana
Copy link
Contributor

iliana commented Nov 18, 2019

@jahkeup The issue is that we're not locking the cargo-deny version :)

v0.4.0 was released November 7 and that updates to v3.7 of the SPDX license list. OpenSSL's license name changed from Openssl to OpenSSL, and I remember seeing some PRs related to fixing the LLVM-exception exception.

Lock to v0.3.0 (and also lock cargo-make to whatever the current version is). Tracking the update to deny.toml in #531

tools/infra/env/bin/logmsg Outdated Show resolved Hide resolved
tools/infra/env/bin/setup-rust-builder Outdated Show resolved Hide resolved
tools/infra/env/bin/retry Outdated Show resolved Hide resolved
tools/infra/env/bin/retry Outdated Show resolved Hide resolved
tools/infra/env/bin/retry Outdated Show resolved Hide resolved
@jahkeup
Copy link
Member Author

jahkeup commented Nov 18, 2019

@iliana
Copy link
Contributor

iliana commented Nov 18, 2019

@jahkeup oh, my local copy is 0.2.6. >_<

Maybe just lock to that for now then.

There's no apparent need for s3:DeleteObject in the context of CodeBuild
operation. Neither the artifact's nor cache's buckets have expirations
performed by CodeBuild.
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

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

🏗

@jahkeup jahkeup merged commit 91cb20a into develop Nov 19, 2019
@iliana iliana added this to the v0.2.0 milestone Nov 20, 2019
@iliana iliana deleted the relauto branch December 10, 2019 21:45
@zmrow zmrow mentioned this pull request Dec 20, 2019
3 tasks
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.

5 participants