darkrenaissance / darkfi

Anonymous. Uncensored. Sovereign.
https://dark.fi
GNU Affero General Public License v3.0
1.07k stars 114 forks source link

Updated a handful of packages to resolve build errors #232

Closed bobsummerwill closed 10 months ago

bobsummerwill commented 10 months ago

Updated a handful of packages to resolve build errors when using the nightly rustup toolset. See https://substrate.stackexchange.com/questions/5379/how-do-i-fix-a-failed-build-error-e0635-unknown-feature-proc-macro-span-shri for the proc2 issue. The second was related to value-bag. In both cases the resolution was to update the versions for problematic packages to use the versions from the master branch.

bobsummerwill commented 10 months ago

This is a diff against the v0.4.1 label, and I see there is no long-standing branch corresponding to that label.

Given how long-running that label has been for ircd, if not for anything else, perhaps such a branch should actually be created.

The build is broken out of the box with nightly (which is what the build instructions say to use).

bobsummerwill commented 10 months ago

There is a per-directly override command for rustup, and some config files. Maybe we can do something with a rust-toolchain-toml?

https://rust-lang.github.io/rustup/overrides.html#directory-overrides

https://rust-lang.github.io/rustup/overrides.html#the-toolchain-file

bobsummerwill commented 10 months ago

To do any of this, though, we'll need a branch for that "stable" tag, so we can at least bug-fix.

aggstam commented 10 months ago

Hello @bobsummerwill ! As per docs You have to install a native toolchain, which is set up during Rust installation, nightly toolchain and wasm32 target. Native refers to stable toolchain, which should be your default one. Tag v0.4.1 should only be build against stable toolchain, not nightly.

bobsummerwill commented 10 months ago

Hey @aggstam,

So I think I see where the instructions are off, which is in the assumption that the user will have a stable toolchain installed already and that the stable toolchain will be the default.

In my case I installed Rustup via a snap on Ubuntu 23.10 and that did not install a stable native toolchain. I only had the nightly toolchain from:

% rustup toolchain install nightly

so that ended up as my default.

There are not actually any instructions about when to use stable and when to use nightly, despite it telling you to install the nightly toolchain. My assumption from that was that nightly was required.

(And I was unaware that I was missing stable too!)

aggstam commented 10 months ago

Hey @bobsummerwill , I don't believe that an assumption exists, since docs clearly state You have to install a native toolchain, which refers to your specific architecture stable toolchain. Most rust installations methods install stable by default, unless specified otherwise. I'm not a snap user so I don't know its behavior, but I reckon not installing a default toolchain is either a user missconfig or them not shipping one by default, which is not the "commom" behavior.

Now regarding toolchain usage, Makefiles are configured to use the appropriate toolchain where needed, no user selection is needed, so if you want to find out what is used where, check those out.

I hope I answered all your questions, happy to hear you thoughts :D

bobsummerwill commented 10 months ago

Aha - I see exactly what has happened here.

The per-project Makefiles were only introduced by @parazyd in August:

https://github.com/darkrenaissance/darkfi/commit/41bf60570ae4a064857b39ebb361040638c51761

...long after that v0.4.1 tag was created in February:

https://github.com/darkrenaissance/darkfi/tree/0766e910aae7af482885d0a5b05ccb61ae7c1af4

The global Makefile back then did not specify either stable or nightly (if I am understanding the CARGO = cargo syntax correctly) so you get whatever your default toolchain is (which was nightly for me because of the Snap weirdness):

https://github.com/darkrenaissance/darkfi/blob/0766e910aae7af482885d0a5b05ccb61ae7c1af4/Makefile

What is more, looking at the current global Makefile, it looks like that global CARGO nightly variable is being forced down on the zkas and darkirc Makefiles anyway, though not onto the other binaries. darkfid, for example, is explicitly setting nightly locally and is NOT being overridden, but darkirc is explicitly setting nightly locally and IS being overridden but with the same setting.

So we cannot retroactively fix v0.4.1 but I think there is perhaps a little more work which can be done here to make things more explicit. I'll create a new PR and try some things out.

Given that we are explicitly specifying toolset per binary I would assume that we would be wanting to use stable toolchain everywhere UNLESS a binary was dependent on some cutting edge feature which has not made it to a stable toolchain yet?