Remove absolute registry paths from panic messages in wasm #1594
Conversation
brson
changed the title
|
One thing to note here is that this affects the builtin file! macro, so if some dependency is doing something funky with that macro it could break in unexpected ways. |
Do you have any examples of how use of a file! macro would be affected? Am I correct in understanding that it'd only affect use of the builtin |
Yes it will only affect paths to the registry, but those paths will no longer be "valid" (absolute or relative), so if some dependency code is doing something particularly unexpected by trying to use I am not aware of any code or use case for actually doing this though - within the rust code base I did a github code search and it seems Edit: From a re-read of cargo's rustflags handling, rustflags may not apply to build scripts at all anyway. |
|
@leighmcculloch I have pushed a commit that does the remapping with One nice advantage is that Disadvantages:
This patch continues to merge any existing I have manually retested the mapping with the eth_abi crate, and tested the warnings. |
I'm wondering how this will affect Windows users. It's been a while since I was a regular windows users, but when I last was it was possible to have spaces in the username and therefore in the home path. 🤔 |
Can the value be escaped? |
|
I pushed a change in 240a6b3 to output the env var to stderr, so that the command that's outputted shows the full command in use. For example: |
There was a problem hiding this comment.
I think this looks good and we can run with this.
A question remains about whether we add tests for this logic. We don't currently test the build command because up until now it was rather uninteresting, and not trivial to test. But as we add more logic, feels like we should do that now.
I'm thinking if we have a small test vector contract, and check that soroban contract build produces a valid wasm file, and then have some tests that run on mac, windows, linux, with the same version of rust, should produce the same wasm file? Maybe that's overkill...
Thoughts?
I do not believe so. I looked but could not find evidence that this is unescaped anywhere. |
I think this is very testworthy but testing it is tough. The idea of comparing across OSes would probably work for a simple contract, though contracts are still not reproducable across OSes generally after this patch (needs to be fixed in cargo). Examining the CI results of each OS like that seems pretty complicated though. Maybe the expected wash hash can be hardcoded (and would have to change as compilers etc change). My preference for testing this particular patch would be to: build a test wasm that is known to contain paths for panic messages; extract those paths from the wasm; check that they are not absolute paths, and that the first path segment in those paths is as expected. Extracting that metadata is definitely possible, but it's quite a big test setup to test this one thing, which is why I didn't bother. I'm happy to go create that test though. Something to hack on. |
|
I fixed the merge conflicts and manually retested. Building after the merge did change the lockfile significantly - I didn't look closely and assume it's correct. |
While this is possible, I believe these paths end of in a big blob of an rodata section, so parsing them out of there could be ugly. |
There was a problem hiding this comment.
I'm going to approve and merge this and we can get feedback.
I think the change is relatively low risk.
But, we may find out that the assumption around spaces in the filesystem path is a large problem in which case maybe in a future version we flip to using another env var to set it.
|
@brson Thanks for the effort to understand what could be done to improve reproducibility and navigate the options and tradeoffs. |
|
I guess we should have asked sooner, a quick ask on Discord (ref) raised a comment that spaces in windows home paths are common because the username is often a "first last" name. I might have pushed us towards the wrong tradeoff. 🤔 |
Oops. Good news though: the result of this should just be that stellar-cli prints a warning on windows if their paths contain a space - they can still develop fine, but can't create a reproducible build. I've filed an issue about testing this feature, as previously discussed, and am working on it: #1704 We can also revisit this implementation, dig through cargo again and see if there is any other way to do it while not clobbering rustflags and handling spaces; maybe see if there is any possibility of accelerating cargo feature work to make this more reliable. In the meantime I am cautiously optimistic that this implementation is not going to cause much heartache. |
|
Agreed, I think it is very unlikely problems would be created, and it'll be helpful to let this settle and to see how this positively influences things. A real hope would be that Rust would stabilise the trim paths RFC that you mentioned on the issue and then we could simply engage it. |
What
This uses the
--remap-path-prefixoption torustcto eliminate absolute paths embedded in the wasm.Fixes #1445
Why
This is generally required to make builds reproducible across different machines.
These absolute paths can appear for both debuginfo and panic messages, though in our case since we are compiling in release mode and stripping, in practice this only affects panic messages.
The comments in the patch are fairly thorough as to why and how the patch works, but there are some things to note.
Known limitations
This relies on setting
CARGO_ENCODED_RUSTFLAGS, which is similar toRUSTFLAGS, and which cargo interprets with higher priority. As of now this is the only way to pass the required flag to all dependencies, and crates.io dependencies are where this problem appears specifically (we can't just pass-- --remap-path-prefixtocargo rustc).By setting one of the RUSTFLAGS vars, cargo ignores any
build.rustflagsand similar configuration options on the local filesystem. Detecting this situation and adapting to it is challenging and probably involves using cargo's own API as a dependency.I do not quite understand the conditions under which the problem this addresses actually occurs, and don't know how to succinctly construct a test case. Panic messages in the local workspace do not exhibit the problem, nor do those in the pre-compiled standard library (which is already compiled in a similar way). Triggering the problem requires using dependencies from crates.io; and possibly crates.io crates that call into the core library where a panic is located - I'm not sure.
I know it occurs for the
eth_abiexample, which depends on some third-party crates.The data section of the
eth_abiwat before and after this patch:I have tested this on windows in addition to linux and mac and it appears to work.