Implement a compile context/subcommand for standalone compilation mode#239
Implement a compile context/subcommand for standalone compilation mode#239
compile context/subcommand for standalone compilation mode#239Conversation
This removes the need for a lot of duplicate code. If adding support for a new specifier, instead of having to add 4 new macros that share almost identical logic with the other macros, only 1 match arm needs to be added to one existing macro. This refactor also decouples the event's specifier field name from the reporter's specifier field name, allowing them to be named differently if needed. I think this also increases the readability somewhat of the macros, showing more clearly what it matches on.
There was a problem hiding this comment.
This file uses the same pattern as the differential tests entry point.
| let compilation_definitions = create_compilation_definitions_stream( | ||
| &full_context, | ||
| &corpus, | ||
| // TODO (temporarily always using `z`): Accept mode(s) via CLI. |
There was a problem hiding this comment.
Will be addressed in the next PR.
There was a problem hiding this comment.
The macros here have been refactored in order to remove duplicate code. If adding support for a new specifier (as in this PR), only 1 match arm now needs to be added to one existing macro (avoiding the need to add 4 new macros that share almost identical logic with the other macros).
This refactor also decouples the event's specifier field name from the reporter's specifier field name, allowing them to be named differently if needed. I think this also increases the readability somewhat of the macros, showing a bit more clearly what it matches on.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| pub enum ParsedCompilationSpecifier { |
There was a problem hiding this comment.
Why do we need this file rather than the TestSpecifier? They seem to be functionally the same but the test specifier seems to be more permissive in what it allows us to point to.
There was a problem hiding this comment.
Grouped my response here to some related comments such as that ☝️ one.
There was a problem hiding this comment.
We discussed this and I agree with you that your approach here is better. Will mark this as resolved.
crates/config/src/lib.rs
Outdated
| default_value = "", | ||
| value_hint = ValueHint::DirPath, | ||
| )] | ||
| pub working_directory: WorkingDirectoryConfiguration, |
There was a problem hiding this comment.
I suggest that we remove the working directory from here. This is because we use it for 1) node files and 2) cached compiler artifacts. For this sub-command we don't use nodes and never want to make use of the cached compiler.
There was a problem hiding this comment.
It's also used for the report location, so since the report currently is added to the working directory, I included that in this config. Do you suggest adding the report somewhere else in the case of the compile command?
There was a problem hiding this comment.
Ah, good point :D I suppose it's okay for the working directory to be provided and we could just invalidate the cached compiler always for the compilation runs.
crates/report/src/reporter_event.rs
Outdated
| }, | ||
|
|
||
| /// An event sent by the reporter once an entire metadata file and mode combination has | ||
| /// finished standalone compilation. |
There was a problem hiding this comment.
Can we refer in the codebase to standalone compilation as pre-link compilation? I think that it follows the same naming convention we've been using in the codebase.
|
|
||
| /// This is a full description of a compilation to run alongside the full metadata file | ||
| /// and the specific mode to compile with. | ||
| pub struct CompilationDefinition<'a> { |
There was a problem hiding this comment.
Hmm... i'm a bit worried about there being a lot of copied code in this file and the fact that we could then have code divergences between these two files. Is there anything we can do about that? Also, we've just had our first code diversion with the pragma solidity check which we've not done so far because we rely on the metadata files containing modes which version of the compiler to use.
There was a problem hiding this comment.
I'll group my response here to some comments that I think are related. That one ☝️ , this and this (ParsedCompilationSpecifier vs ParsedTestSpecifier), and this (CorpusCompilationConfiguration vs CorpusExecutionConfiguration):
Currently, the ParsedTestSpecifier is test execution specific (referring to cases which is not needed in standalone/prelink). The way I see the usage differ from the current test/bench features is that the cases (and their modes) in the metadata files are intended for execution, so the preexisting ParsedTestSpecifier and CorpusExecutionConfiguration make sense, inferring the opt modes from that.
However, for the (standalone/prelink-only) compile command, it could be good to allow passing the modes as CLI args (saying "all compatible files should be compiled with these opt levels"), rather than having to rely on the levels indicated in the metadata files. So if we want to compile with other levels, we don't need to update metadata files, which then modifies what the test/bench features test (and vice versa). The metadata files logic is still used to e.g. find the related files to compile. (Accepting the opt levels via CLI args would be the next PR in that case btw, this PR uses only 1, acting as if the user only provided 1 level.)
I'd love to share more logic between the features, though, where suitable.
Having the compile feature determine modes in this way (not via the metadata files), leads to the divergence you're speaking of with the pragma solidity check. What are your thoughts on this difference in usage?
There was a problem hiding this comment.
Regarding what we briefly discussed offline about potentially using case idx 0 for standalone comp mode, I'd opt for not going with that approach. Essentially because it conflates unrelated concepts and introduces irrelevant data that we need to remember to interpret differently from what would be semantically assumed (e.g. a “case”, whether using a hardcoded index or not, is ambiguous in a standalone compilation context). I think it'd alleviate making test-specific or compile-only specific changes if it's a clearer separation between the two.
Claude and I looked more into possible ways to share more of the code where suitable, where one thing could be e.g. the orchestration logic (concurrent (test and comp) or sequential (bench)).
I'd say that such a refactor could in that case be done in a follow-up (even tho it may be small) once we get this feature out and can start being used by the resolc repo. Thoughts?
There was a problem hiding this comment.
After our discussion, I get why this struct was added and the overall abstraction that you're after. Also, I understand not wanting to do the "case index 0" approach, to be honest it was somewhat of a hacky approach but it was a way to reuse a lot of what we had.
I'd recommend that we would want to reuse more code in this PR since it's already quite a small change to make and there's no need to delay it until a later PR.
I imagine that code-reuse for this could look something like the following:
pub struct SomeDefinition<'a, AdditionalState> {
// There are fields which are shared by the test definitions and the compilation definitions.
shared_field1: SomeType,
shared_field2: SomeOtherType,
// These are fields which are different between the test case definition and the compilation definition.
additional_state: AdditionalState
}Then, we can specialize the implementation of check_compatibility based on which type of state is available
impl<'a> SomeDefinition<'a, TestAdditionalState> {
pub fn check_compatability(&self) -> Result<()> {
self.check_compatability_of_shareed_fields()?;
todo!()
}
}
impl<'a> SomeDefinition<'a, CompilationAdditionalState> {
pub fn check_compatability(&self) -> Result<()> {
self.check_compatability_of_shareed_fields()?;
todo!()
}
}
impl<'a, AdditionalState> SomeDefinition<'a, AdditionalState> {
pub fn check_compatability_of_shareed_fields(&self) -> Result<()> {
todo!()
}
}Additionally, we could implement Deref and Deref mut for this which would mean that to parts of the code using these structs it would look as if their structure was the same since we we're still able to access fields on the additional_state with the syntactic sugar of it looking like we're not accessing it. This is nice because it minimizes how large this refactor can be.
There was a problem hiding this comment.
Interesting, okay good idea 🙂
There was a problem hiding this comment.
I've updated the code to share the orchestration logic which is beneficial. I've further evaluated the sharing of a potential SomeSharedDefinition but I'd say that the added code is likely not necessary, because:
- The only shared fields as of now would be the ones below. No method logic is actually shared.
pub metadata: &'a MetadataFile,
pub metadata_file_path: &'a Path,
pub mode: Cow<'a, Mode>,
pub reporter: Reporter, // via type param- If we were to use a shared struct with e.g. a field for additional context-specific state:
I think having the separate definitions is more favorable at this time, but implementing the shared struct would be quick, so let me know if you still would prefer that approach.
| if deployed_libraries.is_some() { | ||
| reporter | ||
| .report_post_link_contracts_compilation_succeeded_event( | ||
| compiler.version().clone(), | ||
| compiler.path(), | ||
| true, | ||
| None, | ||
| cache_value.compiler_output.clone(), | ||
| ) | ||
| .expect("Can't happen"); |
There was a problem hiding this comment.
Dead code, we're already in a match deployed_libraries > None case.
|
It's looking a lot better, can we get more code re-use before we merge? |
compile context/subcommand for standalone compilation modecompile context/subcommand for standalone compilation mode
Summary
Adds support for a new
compilesubcommand for compiling contracts in standalone/pre-link mode (without any test execution).Additions overview:
CompilecontextPreLinkCompilationSpecificReporterandPreLinkCompilationReportNotes:
-Ozin this PR.Example run command:
retester compile \ --compile /path/to/resolc-compiler-tests/fixtures/solidity/simple \ --compile /path/to/resolc-compiler-tests/fixtures/solidity/complex \ --resolc.path /path/to/resolc \ --solc.version "0.8.33" \ --report.file-name report-compile.json \ --working-directory ./workdir \ --concurrency.number-of-threads 10 \ --concurrency.number-of-concurrent-tasks 100 \ > logs-compile.log \ 2> output-compile.logExample report:
Expand to see an example report
{ "context": { "Compile": { // ... } }, "metadata_files": [ // ... ], "execution_information": { "/path/to/solidity/simple/call_chain/address_size1.sol": { "compilation_reports": { "Y Mz S+": { "status": { "status": "Success", "compiled_contracts_info": { "/path/to/solidity/simple/call_chain/address_size1.sol": { "TestA": { "bytecode_hash": "0x694498b0...", "requires_linking": false }, "TestB": { "bytecode_hash": "0x1fc8c569...", "requires_linking": false } // ... } } // ... } } } }, "/path/to/solidity/simple/immutable_evm/trycatch.sol": { "compilation_reports": { "Y Mz S+": { "status": { "status": "Failure", "reason": "Encountered an error..." // ... } } } }, "/path/to/solidity/complex/defi/starkex-verifier/test.json": { "compilation_reports": { "Y Mz S+": { "status": { "status": "Ignored", "reason": "Source pragma is incompatible with the Solidity compiler version.", "compiler_version": "0.8.33", "incompatible_files": [ { "source_path": "/path/to/solidity/complex/defi/starkex-verifier/FriTransform.sol", "pragma": "^0.6.12" } // ... ] } } } } // ... } }