-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CompilerNode interface #757
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
let mut iter = s.split(|c| c == '-'); | ||
let from = iter | ||
.next() | ||
.ok_or_else(|| "Z".parse::<u64>().expect_err("ParseIntError"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always wondered about this, can't we just return a true std::num::ParseIntError
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I remember it is not possible to create ParseIntError
(see rust-lang/rfcs#1143).
I was actually thinking we can just use ()
as the Err type since it's not that relevant/useful I think. But that would need a separate PR.
let _hashes = command.execute(&exe_path).expect("hash list"); | ||
// get all hashes | ||
let command = CompilerHashCmd::new(&common::test_env(), None); | ||
let hashes = command.execute(&exe_path).expect("hash list"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to debug this kind of data-build/compiler tests in the past few weeks, and I was wondering if we could have the corresponding compiler embedded (eventually) in the test executable? Plus, the other issue is that there's no explicit link to the compiler .exe itself, which means that this test & the compiler .exe can be out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I feel your pain. I either run cargo build
before running any tests. Or simply run make test
.
As for embedding compiler - this test is specifically testing calling an external executable - which is a path that we still support.
The issue is that cargo does not provide binary dependencies. There is an rust-lang/rfcs#3028 to add it which would help us solve this problem.
All this indeed creates problems. Basically you need to at least run cargo build -p compiler-*
for some tests to make sense. This is what we have the build machines configured to do; and make test
does the same thing.
a57dc03
to
a37ea53
Compare
a37ea53
to
3e1710b
Compare
26353c4
to
28d5c48
Compare
Changes:
CompilerNode
interface that is used by all compiler-running binariesAssetRegistry
between compilers within the same processCode dependency graph:
Below illustrates code dependencies of data-compilers and executables that use data-build to trigger those compilers.
data_build::DataBuild
data-build.exe
andeditor-srv.exe
are currently the entry points to building data. They do this throughDataBuild
interface which is responsible for creating the build graph and triggering data compilation in the right order.ubercompiler.lib
data_compiler::compiler_node::CompilerNode
This is an interface used by each binary that does data compilation (such as
compiler-a2b.exe
,ubercompiler.exe
,data-build.exe
,editor-srv.exe
). This will help us treat each binary as a node in a "compilation network" when tackling the distribution of building game data.