[move] Simplify binary version override logic #17718
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
| @@ -168,7 +165,7 @@ pub fn publish( | |||
| let mut serialized_modules = vec![]; | |||
| for unit in modules_to_publish { | |||
| let id = unit.unit.module.self_id(); | |||
| let module_bytes = unit.unit.serialize(bytecode_version); | |||
| let module_bytes = unit.unit.serialize(); | |||
There was a problem hiding this comment.
Why not with_version? Is this test only code?
There was a problem hiding this comment.
This is a convenience method on CompiledUnit that uses serialize_with_version under the hood, but it's just so you don't need to do the whole let mut bytes = vec![]; unit.unit.module.serialize_with_version(unit.unit.module.version, &mut bytes); bytes. So I kinda liked keeping it. But I can remove if you want it gone completely.
There was a problem hiding this comment.
Ah this is on the compiler's unit, not the CompiledModule?
There was a problem hiding this comment.
yep! This is on CompiledUnit :)
external-crates/move/crates/move-ir-to-bytecode/src/compiler.rs
Outdated
Show resolved
Hide resolved
external-crates/move/crates/move-ir-to-bytecode/src/compiler.rs
Outdated
Show resolved
Hide resolved
tzakian
force-pushed
the
tzakian/simplify-binary-version-override-logic
branch
from
92da060
to
df13d18
Compare
| @@ -168,7 +165,7 @@ pub fn publish( | |||
| let mut serialized_modules = vec![]; | |||
| for unit in modules_to_publish { | |||
| let id = unit.unit.module.self_id(); | |||
| let module_bytes = unit.unit.serialize(bytecode_version); | |||
| let module_bytes = unit.unit.serialize(); | |||
There was a problem hiding this comment.
Ah this is on the compiler's unit, not the CompiledModule?
| } | ||
|
|
||
| fn set_module_version(module: &mut CompiledModule, version: Option<u32>) { | ||
| if let Some(version) = version.or_else(get_bytecode_version_from_env) { |
There was a problem hiding this comment.
Hmmmm. Should this be get_bytecode_version_from_env().or(version)?
Or maybe we just nuke get_bytecode_version_from_env since there is a compiler flag?
There was a problem hiding this comment.
Only reason I kept the env as well is because even though there's a compiler flag the flag is not surfaced up into the package system/the move build command. So if you ever wanted to build a package using the existing workflows and specify the version the only way of doing that is with the env variable (at least currently).
Should this be get_bytecode_version_from_env().or(version)
I went back and forth on this lol and which should override the other if both are set. I'm happy either way so fine switching it around if that's preferable.
There was a problem hiding this comment.
I don't really care. Let's maybe just focus on propagating that flag and killing get_bytecode_version_from_env
There was a problem hiding this comment.
I think I'll keep get_bytecode_version_from_env at least in this PR since I think there's some nuance that's going to be needed in terms of how we expose the flag out into the build system (i.e., I don't think it should be exposed by default, but maybe only on debug/dev builds etc). Happy to lead the charge on that though (but in a separate PR :) )
tzakian
force-pushed
the
tzakian/simplify-binary-version-override-logic
branch
from
df13d18
to
9e67a86
Compare
Description
The logic around overriding the binary version output by the compiler was spread out over a number of places, and in general was a bit nasty. This simplifies the logic so the version setting happens in only one place in the code base.
Test plan
Make sure existing tests pass.