Changes to Rust options parsing to facilitate Pants NG#23050
Changes to Rust options parsing to facilitate Pants NG#23050benjyw merged 2 commits intopantsbuild:mainfrom
Conversation
benjyw
added
the
release-notes:not-required
|
|
||
| // Value doesn't implement Eq and Hash because of floats, but in our case we can insist that | ||
| // NaN == NaN for config equality/hash purposes. For one thing, we don't meaningfully | ||
| // support NaN in config values (although we don't enforce that they don't appear). |
There was a problem hiding this comment.
Although we don't enforce that they don't appear
Should we? Or at the very least debug assert it?
There was a problem hiding this comment.
Maybe, but we're no worse off than before if we don't. You could put a NaN in pants.toml today.
We have few if any float-valued options though, I am almost inclined to get rid of them one day and just require integers.
| && a0 | ||
| .iter() | ||
| .zip(a1.iter()) | ||
| .all(|(v0, v1)| _eq_toml_value(v0, v1)) |
There was a problem hiding this comment.
Note to self - all is short circuiting.
| Value::Integer(i) => i.hash(state), | ||
| Value::Float(f) => state.write_u64(f.to_bits()), | ||
| Value::Boolean(b) => b.hash(state), | ||
| // We don't support datetime types in config, so we won't incur this to_string() cost |
There was a problem hiding this comment.
It datetime not supported by convention? Or like, we don't parse them earlier in the pipe?
There was a problem hiding this comment.
We don't have accessors for them, so if you tried to read a field with that value type at the TOML level you'd have to read it as a string and I'm not sure what would happen.
| let has_provided_configs = config_sources.is_some(); | ||
|
|
||
| let buildroot = buildroot.unwrap_or(BuildRoot::find()?); | ||
| let buildroot = match buildroot { |
There was a problem hiding this comment.
What's the purpose of this change?
Also, probably should have been unwrap_or_else originally would be my guess?
There was a problem hiding this comment.
To not incur the cost of BuildRoot::find when buildroot is Some(). Could also have done unwrap_or_else(), yeah. I can change if you prefer.
There was a problem hiding this comment.
Oh but then propagating the Err case gets annoying. So that's why I did it this way.
| .file_stem() | ||
| .unwrap() | ||
| .to_string_lossy() | ||
| .strip_prefix("pantsng_") |
There was a problem hiding this comment.
These "tags" are the _42 or _arm64 or _42_arm64 kinda thing? Not what Pants currently refers to as tags? Or do the concepts get merged?
There was a problem hiding this comment.
Actually, this would be a good time to enforce limits on how these are created - what permutations are allowed, underscore vs dots, etc.
There was a problem hiding this comment.
Correct, these are unrelated to those tags, but will likely supersede them.
Currently you can't specify your own tags, there are just a predetermined set of them based on OS, arch etc. But you're right that this needs to be made rigorous before allowing user tags.
| hasher.update(k.as_bytes()); | ||
| for v in vals { | ||
| if let Some(v) = v { | ||
| hasher.update(b"Some"); |
There was a problem hiding this comment.
What is the purpose of adding this to the hash?
There was a problem hiding this comment.
For the case when the bytes happen to spell None... Gotta be explicit.
sureshjoshi
left a comment
sureshjoshi
left a comment
There was a problem hiding this comment.
I added some comments for my own sake, or just clarifying intent.
Since this is still in-prog, I didn't want to go down the rabbit hole of some of the memory/performance/error impacts (e.g. Arc<Mutex<... in a few places).
Also, my general rule of thumb to show intent is that most of my unwrap() are expect with the message for easier debugging when I crash with them.
2 participants
Hashpuborpub(crate)so they can be used by ng code.pantsng_*.tomlwhen running in "ng" mode.