Use OUT_DIR when building

[LLFourn]

In build.rs you should only ever write to OUT_DIR. Currently we are writing to CARGO_HOME. This is annoying because using cache in CI seems to break this for me because that part of CARGO_HOME is not saved.

Probably same treatment needs to be applied to electrum crate too.

[LLFourn]

I was a little confused about what the actions was doing so I did a00c919 which I think was what was intended?

[RCasatta]

I know the recommendation is the build script should place the output into $OUT_DIR, however, it's a problem during development because any change in the code would trigger a new download of the binary. Also, I am not sure how to provide a path like /debug/build/bitcoind-a346178cf7f7c17a/out/bitcoin/bitcoin-0.21.1/bin/bitcoind via downloaded_exe_path() since the docs says $OUT_DIR is provided only during compilation time.

This is annoying because using cache in CI seems to break this for me because that part of CARGO_HOME is not saved.

It should be saved in the cache?https://github.com/bitcoindevkit/bdk/blob/e80be49d1e75e907b6e91049b6a80cf9501c65d0/.github/workflows/cont_integration.yml#L98

I was a little confused about what the actions was doing so I did a00c919 which I think was what was intended?

I think the CI is right. Tests are launched both by using the feature or by using the BITCOIND_EXE environment variable (which take priority if set). The if in the action is needed only for the test with the env var because test test_multi_wallet is disabled automatically with the feature.

[LLFourn]

I know the recommendation is the build script should place the output into $OUT_DIR, however, it's a problem during development because any change in the code would trigger a new download of the binary.

Any change to this library will trigger it but not users of this library which is what matters. For me even changing the features back and forth did not require re-downloading anything.

Also, I am not sure how to provide a path like /debug/build/bitcoind-a346178cf7f7c17a/out/bitcoin/bitcoin-0.21.1/bin/bitcoind via downloaded_exe_path() since the docs says $OUT_DIR is provided only during compilation time.

See the PR which handles this using env! macro which embeds the value at compilation time.

It should be saved in the cache?https://github.com/bitcoindevkit/bdk/blob/e80be49d1e75e907b6e91049b6a80cf9501c65d0/.github/workflows/cont_integration.yml#L98

Right but I am not using this crate from bdk and so don't have those magic lines in my ci to make it work. It would be better if we didn't have to add them. See https://github.com/LLFourn/gun/blob/master/.github/workflows/test.yml#L38 which works very effectively with this PR.

I think the CI is right. Tests are launched both by using the feature or by using the BITCOIND_EXE environment variable (which take priority if set). The if in the action is needed only for the test with the env var because test test_multi_wallet is disabled automatically with the feature.

Ahh ok sorry I can see that what I did makes no sense now and why the BITCOIND_EXE does not point to the right place. I think the right thing to do is to just download bitcoind once to somewhere sensible and run the tests against that using BITCOIND_EXE.

[LLFourn]

Note also that CI is failing on master: https://github.com/LLFourn/bitcoind/actions/runs/1133912807 (this is just a fork of master).

This PR should fix it but I thought I'd let you know.

[RCasatta]

Any change to this library will trigger it but not users of this library which is what matters. For me even changing the features back and forth did not require re-downloading anything.

right, nice.

However, there are still some advantages during development in using CARGO_HOME when doing cargo clean or when using multiple repos using bitcoind crate.

See the PR which handles this using env! macro which embeds the value at compilation time.

I see it works, but I am afraid it's not always granted since the doc says the contrary? see
"(Only set during compilation.)" in https://doc.rust-lang.org/cargo/reference/environment-variables.html

Note also that CI is failing on master: https://github.com/LLFourn/bitcoind/actions/runs/1133912807 (this is just a fork of master)

Oh! I missed that, and I am not sure why since this https://github.com/RCasatta/bitcoind/actions/runs/1101263680 should be the same and worked. Do you know why this PR fixed it?

Anyway, I am inclined to accept the PRs if it's more standard and helps downstream, just wanted to double-check everything.

@LeoComandini I know you are using this crate, what do you think?

[RCasatta]

let's remove these lines for now, we may reintroduce the test with BITCOIND_EXE later on

[RCasatta]

There is a reference to CARGO_HOME in the Readme, could you please also update there?

[notmandatory]

ACK with changes suggested by @RCasatta

I agree it would be nice to keep downloaded bitcoind in CARGO_HOME, but it does seem to violate where build.rs is allowed to put its outputs.

[LLFourn]

Addressed the above comments. Thanks. CI seems be bugged on my branch (one of the macos jobs is stuck) but otherwise happy with this.

Any change to this library will trigger it but not users of this library which is what matters. For me even changing the features back and forth did not require re-downloading anything.

right, nice.

However, there are still some advantages during development in using CARGO_HOME when doing cargo clean or when using multiple repos using bitcoind crate.

Yes using CARGO_HOME leads to less stuff being downloaded which is a definite plus. For me as a developer this is not important as things working as expected. At the end of the day you can just set BITCOIND_EXE right. There is also changing the API so you can use a docker image as a backend rather than an exe.

See the PR which handles this using env! macro which embeds the value at compilation time.

I see it works, but I am afraid it's not always granted since the doc says the contrary? see
"(Only set during compilation.)" in https://doc.rust-lang.org/cargo/reference/environment-variables.html

Right it's only set during compilation which is what we want. It embeds wherever the OUT_DIR is into the compiled crate without us having to care about where that is at runtime.

Note also that CI is failing on master: https://github.com/LLFourn/bitcoind/actions/runs/1133912807 (this is just a fork of master)

Oh! I missed that, and I am not sure why since this https://github.com/RCasatta/bitcoind/actions/runs/1101263680 should be the same and worked. Do you know why this PR fixed it?

I think by making sure certain directories exists before using it.

[RCasatta]

Thanks!