Re-run the local codegen if DSL or parser-generator changes

[Xanewok]

Changing the parser generator code obviously affects the resulting build, so make sure we re-run the build script then.

This fixes the case where modifying the code in PG does not re-run the codegen, e.g. changing the code in quote! macros or other logic.

[changeset-bot]

⚠️ No Changeset found

Latest commit: a894bef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[OmarTawfik]

not sure I understand why is that needed. This crate explicitly imports and uses codegen_parser_generator::code_generator::CodeGenerator, so it would be rebuilt if the source changed. Is something else missing?
I'm hoping to avoid adding these additional/explicit dependencies if we can. I wonder what else is missing?

[Xanewok]

This is because the build pipeline is non-standard and there's indirection involved. This main.rs is the bin target of solidity_cargo_build and any changes to the dependencies marks this binary as dirty.

However, in the build script target of slang_solidity (crates/solidity/outputs/cargo/crate/Cargo.toml) we simply call the binary via infra run solidity_cargo_build and it relies on solidity_cargo_build (bin) to emit correct rerun-if-changed metadata, otherwise Cargo doesn't know that changing the solidity_cargo_build should affect the resulting crate.

We could try and mark this as a path = ... dev-dependency, which are "commented out" at publish-time by cargo. This might create a direct link for our build system so that we don't need to rely on the custom markers so much but still make sure the resulting crate/artifacts do not link to our internal crates in our build pipeline.

WDYT?

EDIT: Nope, dev-dependency does not work as it starts from the lib target; even if we add a dummy solidity_cargo_build (lib), the build script is still not re-run. I managed to finally get the correct behaviour by specifying

# crates/solidity/outputs/cargo/crate/Cargo.toml
[build-dependencies]
solidity_cargo_build = { path = '../build' }

but judging by the comments in the indirect cargo/build/src/main.rs that's exactly what we don't want.. I can't seem to find an immediate solution to this problem short of explicitly marking these dependencies or by changing the way we generate these package artifacts.

[OmarTawfik]

Thanks for clarifying. This whole "non-standard" build pipeline is because Cargo doesn't support dependencies that are both [build-dependencies] and [dev-dependencies]. This tiny issue made me lose more time than I would like to admit trying to work around it :(

could try and mark this as a path = ... dev-dependency, which are "commented out" at publish-time by cargo.

The last time I attempted that, I discovered that [dev-dependencies] are not rebuilt/checked during cargo check (or Rust Analyzer intellisense). It is only rebuilt/checked during cargo test and a few other commands that need to build code, not just check it :(
If you have any ideas on how to make [dev-dependencies] work with cargo check that would be wonderful! and it will allow us to deprecate a few more workarounds like this.

If not, and we absolutely have to mark these crates explicitly, I suggest:

1. Combining them with the similar check below for solidity_cargo_build and solidity_npm_build.
2. Only checking Cargo.toml and src directory, so that we don't pick up changes to target or other ignored/temp files.
3. Do we also need to include codegen_grammar in these checks?

Maybe something along the lines of:

for crate_name in &[
    // Instruct the caller source crate to rebuild itself if the this build crate changes:
    "solidity_cargo_build",
    // The parser generator itself affects the build, so make sure we rerun the build script if it changes:
    "codegen_parser_generator",

    ...others?
] {
        let crate_dir = CargoWorkspace::locate_source_crate(crate_name)?;

        rerun_if_changed!(crate_dir.join("Cargo.toml").unwrap_str());
        rerun_if_changed!(crate_dir.join("src").unwrap_str());
}

WDYT?

[Xanewok]

Cargo doesn't support dependencies that are both [build-dependencies] and [dev-dependencies].

Could you elaborate on that?

I did some research and to achieve what we have already there's not many options we can follow.

We want to separate the resulting crate artifacts from the build pipeline.

We could, in theory, also publish slang-the-intermediate-machinery but I understand that it's very experimental, we want to keep it private and so on. This makes sense and is a reasonable assumption.

This, realistically, limits us to using a dev-dependency at most (path-based ones are stripped at package-time).

The previous approach of a separate build.rs-only solidity_cargo_build crate (I'm going to skip the npm variant since they share the the concept) was good for separating the build pipeline from the artifacts, with the exception of...

We want to run the codegen before we even consider checking/testing the resulting binary.

This is tricky, because separating the build step into separate crate severs the dependency trail from the resulting crate that we need to know whether we need to re-run the codegen, which might further alter the crate itself.

The current solution of (only locally) running a cargo run-as-build-script inside the actual build script for the resulting package accomplishes the above goal, but not without issues: we are working around build deadlocks (i.e. rust-lang/cargo#6412, potentially solved with unstable -Z bindeps) and the dependency info is very crude and manual/prone to omission (e.g. this PR).

Unfortunately, using a dev-dependency cannot help us here, since it's only compiled in for (and thus, dependent on by) test targets and examples (marked with an asterisk). Roughly, this is what the unit targets are for a Cargo package:

                                             ,.----> integration tests*
                                             |
build script --->   lib    ------------------+-----> bins --each--> bins* (#[cfg(test)])
                     |                       |
                     L-> lib* (#[cfg(test)]) L---- > examples*

Since the core lib target does not depend on a dev-dependency, it's checked as soon as possible by cargo and will attempt to check stale generated code, before the codegen starts (if the package didn't have a build script with a codegen step, like we do now).

Having the above in mind, we could:

1. keep using the existing mechanism (maybe fix the deadlock once -Z bindeps stabilises) and try to keep our explicit dependency information as precise as possible (e.g. this PR) or we might
2. use a local-only build-dependency for the resulting package, which should be stripped when published. We have our custom publish pipeline in CI, so we could annotate the local-only dependency and rip it out then.

The alternative requires a bit more maintenance in the CI/CD workflow but would simplify the build process and would make sure it's correct.

I think I lean slightly towards the latter since it'd be more obvious for me what's happening (running a binary having a custom target dir as build script inside an actual build script seems non-obvious), and probably more familiar to other Rust devs, as the pattern of patching the manifest before the publish was so widespread wrt dev-dependencies at some point that it was upstreamed to Cargo.

[OmarTawfik]

I solved the deadlock issue by having separate target directories for these build scripts. We end up building some things twice (slower build), but no deadlocks/issues so far. Maybe we can use the solution above when it is ready.

[OmarTawfik]

So, patching seems like the better way to go moving forward. Thanks for checking!
I’m not sure I’ve time to look into it soon, but please feel free to take a stab at it if you want.

Otherwise, your solution now looks great for the meantime. Just the suggestions above:

If not, and we absolutely have to mark these crates explicitly, I suggest:

1. Combining them with the similar check below for solidity_cargo_build and solidity_npm_build.
2. Only checking Cargo.toml and src directory, so that we don't pick up changes to target or other ignored/temp files.
3. Do we also need to include codegen_grammar in these checks?

Maybe something along the lines of:

[Xanewok]

I solved the deadlock issue (..)

I know (hence the link to the Cargo's #6412) 😄 Just wanted to mention -Z bindeps for completeness' sake as a possible more principled fix in the future if we keep using this approach.

I'll add the dependencies manually for now and we can revisit it in a follow-up at some point, if it's okay 👍

[Xanewok]

Added in 610a825

[OmarTawfik]

Left one question

[Xanewok]

Updated as per #597 (comment) and #597 (comment), this should be ready for review again.

I verified that this fixes the issue - previously modifying the dsl.rs or PG code would not re-run the codegen, whereas now it does on cargo check (or e.g. infra test).