fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. #4331

chefsale · 2021-05-27T20:22:17Z

builds to fail:
https://buildkite.com/nearprotocol/nearcore-release/builds/601

Created a build to verify if it works as a quick fix to unblock master failing: https://buildkite.com/nearprotocol/nearcore-release/builds/607

As well remove the mount=type=cache as they get removed eitherways every time once we destroy the container, so there aren't useful at all here.

chefsale · 2021-05-27T21:46:52Z

cc: @matklad to take a look and help 😄

frol · 2021-05-28T07:38:01Z

@chefsale it should not really use +stable in the command line. Can you remove it and try just cargo build --package neard --release?

P.S. It should either use +$(cat rust-toolchain) like our Dockerfile does or let cargo figuring it out itself.

UPD: I don't believe we want to revert resolver

chefsale · 2021-05-28T07:53:22Z

OK @frol , yes I agree. I will add the cat rust toolchain as the dockerfile does shortly.

chefsale · 2021-05-28T08:09:01Z

@frol changed the makefile to use the toolchain provided in the file, didn't realize it wasn't the case there, this seems to solve the build release issue: https://buildkite.com/nearprotocol/nearcore-release/builds/607#f6e8f42b-a385-4dd9-9662-c70eb57d049a

chefsale · 2021-05-28T08:10:09Z

The issue with docker build still persist after we change with nearcore and neard split, for some reason it runs longer than an hour and it times out.

…er conflit, remove the dockerfile caching as it is not used anyway.

chefsale · 2021-05-28T08:22:22Z

Added verbose logging for the docker build and increased timeout to 10 hours to see if they build takes longer for some reason and what are the logs saying: https://buildkite.com/nearprotocol/nearcore-release/builds/609

matklad · 2021-05-28T11:32:15Z

Looking into this now.

Good build: https://buildkite.com/nearprotocol/nearcore-release/builds/600#b6617b72-bed8-4f49-aab3-a3c0202c41db
Bad build: https://buildkite.com/nearprotocol/nearcore-release/builds/609#c54f348e-1301-41da-8ca6-fde8b5158a29

Seems that the build is just stuck for some reason 🤔. I've tried running a couple of clean builds locally without docker:

old a668e43

λ t cargo build -q --release -p neard
real 347.37s
cpu  2719.58s (2611.37s user + 108.21s sys)
rss  3595.57mb

new 4009a14

λ t cargo build -q --release -p neard
real 289.45s
cpu  2681.82s (2574.72s user + 107.10s sys)
rss  3537.24mb

So far, no clue as to what's going on

chefsale · 2021-05-28T11:51:17Z

So regular build works now, but the docker build takes 3 plus hours so far, it is running though. It could be issue in the dockerfile also I believe, one thing is we shouldn't be using the volume at all, as it slows things down. But could be something else as well, but most likely due to something with docker:

https://buildkite.com/nearprotocol/nearcore-release/builds/609

matklad · 2021-05-28T11:57:42Z

I think I've reprodced the issue in a local docker build (it also seems like it's stuck compiling nearcore), investigating .

chefsale · 2021-05-28T12:00:55Z

Yes, it gets stuck on:

[2021-05-28T08:32:22Z] #14 312.9    Compiling nearcore v1.2.0 (/near/nearcore)

for hours 😅

matklad · 2021-05-28T12:46:02Z

I think debugged this. What actually hangs is near-test-contrats/build.rs. The reason it hangs is that it invoked cargo build to build iteslf, and, as we set CARGO_TARGET in docker, we end up in a situation where two cargo invocations share the same build directory, resulting in a deadlock.

The reasonable question to ask is "why we are at all building test contracts in the process of building neard?". This because neard, for some reason, depends on testlib as a non-dev dependency.

matklad · 2021-05-28T13:05:30Z

will prepare a fix later today

chefsale · 2021-05-28T13:58:32Z

@matklad you are the best. Can I get an approval on this changes for now and than leave this docker fix to you? cc: @frol @mhalambek

matklad · 2021-05-28T14:00:10Z

The change looks good to me. Although, rather than using + syntax, I'd just set RUSTUP_TOOLCHAIN env var, but that's not really relevant.

matklad · 2021-05-28T15:28:36Z

fix: prevent build-time deadlock when building tests in release mode #4332 is the minimal fix
fix: remove test code from neard #4333 is the fix for the underlying root cause (compiling test code into neard)

…4332) See #4331 Test Plan --------- This affects only build code for our tests, so just usual release pipeline is enough. Note that we currently do build test code into the released binary, this will be dealt with separately.

matklad · 2021-05-28T18:01:31Z

Stable release got through: https://buildkite.com/nearprotocol/nearcore-release/builds/611#acae2d30-1188-4731-8453-fde4df42d6e9

Nightly is still broken though, will look into that next week .

@ailisp

We're sort of in a weird spot as it is, we have features within `nearcore` that are required but can't be enabled if they depend on crates listed under `dev-dependencies` (bug in cargo: rust-lang/cargo#6915). Temporarily listing them under `dependencies`, fixed the CI testing failures we had while working on #4292 (same error @ailisp highlighted: #4333 (comment)), but as pointed out in #4331 (comment), this method should be out of the question. If we remove it, however, we can't work with the tests that depend on those features. This PR moves the _previously top-level_ tests into a new crate to have better dependency and feature handling as @matklad suggested > * [move tests from `/test` folder somewhere -- either to some of the existing packages (neard perhaps?) or into a new `integration-tests` package](#4292 (comment)) This would ensure we have dependencies like `testlib`, `runtime-params-estimator` and `restored-receipts-verifier` that are only needed for testing and depend on nearcore without cyclic dependency issues while having all features relevant to testing in order. The features within `neard` have been reduced to proxies to `nearcore` features, which partially resolves #4325 (though the intent for that has morphed a bit). This also makes for a cleaner dependency graph from `neard` perspective, removing extraneous dependencies that were previously required. I also noticed removed the `old_tests` feature that should've been removed along with the rest of it in #928. Updated some docs and code comments referencing the old `neard` path too.

chefsale requested review from miraclx, frol, mhalambek and bowenwang1996 May 27, 2021 20:22

chefsale self-assigned this May 27, 2021

chefsale added T-SRE Team: issues relevant to the SRE team automerge labels May 27, 2021

chefsale force-pushed the remove-resolver branch from 5644157 to 1dd1331 Compare May 27, 2021 22:39

chefsale force-pushed the remove-resolver branch 2 times, most recently from 16d730d to 40310e2 Compare May 28, 2021 08:02

chefsale changed the title ~~fix(release): the resolver = 2 feature is a nightly feature causing t…~~ fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. May 28, 2021

fix(release): fix broken release due to wrong rust version and resolv…

947965b

…er conflit, remove the dockerfile caching as it is not used anyway.

chefsale force-pushed the remove-resolver branch from 40310e2 to 947965b Compare May 28, 2021 08:16

matklad mentioned this pull request May 28, 2021

fix: prevent build-time deadlock when building tests in release mode #4332

Merged

Merge refs/heads/master into remove-resolver

cfda09e

bowenwang1996 approved these changes May 28, 2021

View reviewed changes

near-bulldozer bot merged commit 928f15d into master May 28, 2021

near-bulldozer bot deleted the remove-resolver branch May 28, 2021 17:29

miraclx mentioned this pull request May 29, 2021

fix(neard): fixed failed CI tests due to features mismatches #4339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. #4331

fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. #4331

chefsale commented May 27, 2021 •

edited

Loading

chefsale commented May 27, 2021

frol commented May 28, 2021 •

edited

Loading

chefsale commented May 28, 2021

chefsale commented May 28, 2021

chefsale commented May 28, 2021 •

edited

Loading

chefsale commented May 28, 2021

matklad commented May 28, 2021

chefsale commented May 28, 2021 •

edited

Loading

matklad commented May 28, 2021

chefsale commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

chefsale commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. #4331

fix broken release due to wrong rust version and resolver conflit, remove the dockerfile caching as it is not used anyway. #4331

Conversation

chefsale commented May 27, 2021 • edited Loading

chefsale commented May 27, 2021

frol commented May 28, 2021 • edited Loading

chefsale commented May 28, 2021

chefsale commented May 28, 2021

chefsale commented May 28, 2021 • edited Loading

chefsale commented May 28, 2021

matklad commented May 28, 2021

chefsale commented May 28, 2021 • edited Loading

matklad commented May 28, 2021

chefsale commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

chefsale commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

matklad commented May 28, 2021

chefsale commented May 27, 2021 •

edited

Loading

frol commented May 28, 2021 •

edited

Loading

chefsale commented May 28, 2021 •

edited

Loading

chefsale commented May 28, 2021 •

edited

Loading