rustwide_builder: allow using a try build

[jsha]

Followup from #1889 (comment)

[jsha]

I'm confused about the clippy errors, which seem unrelated to my change. Maybe there's a new version of clippy that triggers on some things it didn't before?

[GuillaumeGomez]

The new release was yesterday so new clippy lints available now. ;)

[Nemo157]

Ah, I was confused why my pre-fixing clippy lints as they appeared on nightly didn't catch these. I don't use --all-targets locally and these were in the test code, I'll remember to use it next time I do that (there's already some new warnings on the latest nightly clippy triggering).

[syphar]

@jyn514 is something missing to merge this?

[jyn514]

@syphar not that I'm aware of - haven't had the chance to test it locally, but hopefully @jsha has tested it. It would be nice to avoid the workaround by fixing rustwide but we shouldn't block on that, this is too useful haha

[syphar]

@jsha just in case: did you run tests with this code specifically?

[jsha]

Yes, I've run cargo test with DOCSRS_TOOLCHAIN=6703d43029387eef125596dd7071d0d4fab902c3 in the env on this branch.

[syphar]

Yes, I've run cargo test with DOCSRS_TOOLCHAIN=6703d43029387eef125596dd7071d0d4fab902c3 in the env on this branch.

Cargo test won't test the build, can you check cargo test --ignored?

[syphar]

cc @jsha just as a last check, to be safe: did you run cargo test --ignored with the try-build? so the build-tests run?

[jsha]

I ran it after your suggestion (thanks!) and it failed, but I haven't had a chance to dig into why.

[syphar]

I ran it after your suggestion (thanks!) and it failed, but I haven't had a chance to dig into why.

but the manual build with this code worked?

Just a thought: (I don't know much about try-builds), but couldn't we just use some specific try-build that exists for longer, and add a testcase testing if running the build works with it?

[jsha]

but the manual build with this code worked?

Yep.

Just a thought: (I don't know much about try-builds), but couldn't we just use some specific try-build that exists for longer, and add a testcase testing if running the build works with it?

This is a good idea. I'll have to check how long they last.

Here's the error I'm getting from cargo test -- --ignored:

2022-11-13T09:01:54.990157Z  INFO log: [stderr] error: failed to create directory `/opt/rustwide/target/debug` log.target="rustwide::cmd" log.module_path="rustwide::cmd" log.file="/home/jsha/.cargo/registry/src/index.crates.io-e139d0d48fed7772/rustwide-0.15.1/src/cmd/mod.rs" log.line=621
2022-11-13T09:01:54.990222Z  INFO log: [stderr]  log.target="rustwide::cmd" log.module_path="rustwide::cmd" log.file="/home/jsha/.cargo/registry/src/index.crates.io-e139d0d48fed7772/rustwide-0.15.1/src/cmd/mod.rs" log.line=621
2022-11-13T09:01:54.990253Z  INFO log: [stderr] Caused by: log.target="rustwide::cmd" log.module_path="rustwide::cmd" log.file="/home/jsha/.cargo/registry/src/index.crates.io-e139d0d48fed7772/rustwide-0.15.1/src/cmd/mod.rs" log.line=621
2022-11-13T09:01:54.990279Z  INFO log: [stderr]   Permission denied (os error 13) log.target="rustwide::cmd" log.module_path="rustwide::cmd" log.file="/home/jsha/.cargo/registry/src/index.crates.io-e139d0d48fed7772/rustwide-0.15.1/src/cmd/mod.rs" log.line=621

[jyn514]

All commit artifacts are deleted every 6 months. If you want to update the try commit manually every 5 months that doesn't seem unreasonable, but given that this is just for local development I think it's ok to just not test it too.

[jsha]

This is interesting. If I run just one of the build tests at a time, they pass. For instance each of these passes:

cargo test -- --ignored test_build_binary_crate 
cargo test -- --ignored test_build_crate

But this fails:

cargo test -- --ignored test_build_binary_crate test_build_crate

And this passes:

cargo test -- --ignored test_build_binary_crate test_build_crate --test-threads 1

So it seems like some sort of concurrency issue, with different threads stomping on each other.

[syphar]

All commit artifacts are deleted every 6 months. If you want to update the try commit manually every 5 months that doesn't seem unreasonable, but given that this is just for local development I think it's ok to just not test it too.

Ah, I didn't know that.

Then yes, manual test is fine.

[jsha]

So it seems like some sort of concurrency issue, with different threads stomping on each other.

Scratch that. I realized I was actually running into this concurrency issue with DOCRS_TOOLCHAIN=nightly, which suggests it's a separate, preexisting issue.

Once I switched back to DOCSRS_TOOLCHAIN=6703d43029387eef125596dd7071d0d4fab902c3 I get this error (which I have seen before when I was correctly setting DOCSRS_TOOLCHAIN):

cargo test -- --ignored test_build_crate
...
thread 'docbuilder::rustwide_builder::tests::test_build_crate' panicked at 'called `Result::unwrap()` on an `Err` value: failed to remove '.workspace/builds' : Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', src/docbuilder/rustwide_builder.rs:876:58

Also, I realized the rustwide build tests take their environment from the TestEnvironment, so I would expect them to actually ignore environment variables. Gonna need to do a little more digging.

[syphar]

So it seems like some sort of concurrency issue, with different threads stomping on each other.

Scratch that. I realized I was actually running into this concurrency issue with DOCRS_TOOLCHAIN=nightly, which suggests it's a separate, preexisting issue.

just fyi: in CI we're running the build-tests also with test-threads=1:

docs.rs/.github/workflows/ci.yml

Line 118 in 161dba0

$f --ignored --test-threads=1 || exit 1

[jsha]

I realized the rustwide build tests take their environment from the TestEnvironment, so I would expect them to actually ignore environment variables.

Figured that out. The test env does call from_env, and then overrides a handful of values:

docs.rs/src/test/mod.rs

Lines 258 to 283 in 161dba0

	pub(crate) fn base_config(&self) -> Config {
	let mut config = Config::from_env().expect("failed to get base config");
	// create index directory
	fs::create_dir_all(config.registry_index_path.clone()).unwrap();
	// Use less connections for each test compared to production.
	config.max_pool_size = 4;
	config.min_pool_idle = 0;
	// Use the database for storage, as it's faster than S3.
	config.storage_backend = StorageKind::Database;
	// Use a temporary S3 bucket.
	config.s3_bucket = format!("docsrs-test-bucket-{}", rand::random::<u64>());
	config.s3_bucket_is_temporary = true;
	config.local_archive_cache_path =
	std::env::temp_dir().join(format!("docsrs-test-index-{}", rand::random::<u64>()));
	// set stale content serving so Cache::ForeverInCdn and Cache::ForeverInCdnAndStaleInBrowser
	// are actually different.
	config.cache_control_stale_while_revalidate = Some(86400);
	config
	}

We should probably make TestEnvironment more hermetic by setting all (or almost all?) of the fields, but for now I've let it be.

With the latest pushed commit, this now passes the crate build tests with DOCSRS_TOOLCHAIN=6703d43029387eef125596dd7071d0d4fab902c3.