fix: windows #2132

onbjerg · 2022-06-27T06:46:51Z

After the svm-rs fix we now have new failing tests. Also adds a new test for #2092 and #2085, and will (eventually) fix those bugs

Closes #2078, closes #2092, closes #2085

onbjerg · 2022-06-27T08:01:36Z

Upon more investigation it seems the tests fail because we try to match run tests using ArtifactId::identifier as keys, and on Windows these use , whereas on other platforms they use /

mattsse · 2022-06-27T12:02:10Z

using ArtifactId::identifier as keys,

Where exactly? Where do we perform comparisons with "identifiers"?

onbjerg · 2022-06-27T12:12:24Z

@mattsse In MultiContractRunner we key the results sent back to the test reporter by ArtifactId::identifier here:

foundry/forge/src/multi_runner.rs

Line 287 in 9dcc095

let identifier = id.identifier();

In the multirunner tests we collect these results in a BTreeMap and try to compare them with what we expect. The expected results are stored in a BTreeMap, again, keyed by ArtifactId::identifier:

foundry/forge/src/multi_runner.rs

Line 488 in 9dcc095

"core/FailingSetup.t.sol:FailingSetupTest",

mattsse · 2022-06-27T12:24:33Z

I see, but this is only an issue with the test, right? because on windows, everything should be \?

we can update the tests with either format!("src{}..", MAIN_SEPARATOR)

See https://doc.rust-lang.org/std/path/constant.MAIN_SEPARATOR.html

I think it's also worthwhile adding a function to ArtifactId that does convert slashes, via path-slash crate

onbjerg · 2022-06-27T12:42:40Z

If that's the case then I think that might be our bug for the other Windows issues - we use dunce::canonicalize heavily which will turn \ into / but that's not the case for some ethers-rs stuff. At least that's my current thinking, will have to investigate more. Ideally we'd support both but that also means settling on one version. Afaik Windows supports both

onbjerg · 2022-06-28T16:03:49Z

Currently failing things:

ANSI escape sequences in our snapshots, but those are not present on Windows
Cast crashes because of a stack overflow for some reason (Forge doesn't lol?)
Slashes in remappings are wrong
Slashes in some snapshots are wrong (from ArtifactId::identifier)
Some forge script integration tests fail with "The system cannot move the file to a different disk drive. (os error 17)" whatever that means
One contract won't compile(???)

onbjerg · 2022-07-03T01:21:08Z

cli/src/cmd/forge/script/build.rs

@@ -99,7 +99,7 @@ impl ScriptArgs {

        let mut extra_info = ExtraLinkingInfo {
            no_target_name,
-            target_fname,
+            target_fname: target_fname.clone(),


This clone is for a better error message further down.. Once we clean up linking we should consider cleaning up the targetting logic in script as well (and other commands that take <path>:<name>)

onbjerg · 2022-07-03T01:21:58Z

cli/src/cmd/forge/script/build.rs

+                    let (path, name) = extra
+                        .target_fname
+                        .rsplit_once(':')
+                        .expect("The target specifier is malformed.");


Turns out the bug was staring us in the face all along - on Windows paths obviously contain : so the path we compared with in this branch was literally just the drive letter on Windows lmao

onbjerg · 2022-07-03T01:22:14Z

cli/src/cmd/forge/script/build.rs

@@ -210,22 +211,25 @@ impl ScriptArgs {

        // We received `contract_path:contract_name`
        if let Some(path) = contract.path {
-            let path = dunce::canonicalize(&path)?;
+            let path =
+                dunce::canonicalize(&path).wrap_err("Could not canonicalize the target path")?;


Some error message improvements

onbjerg · 2022-07-03T01:22:57Z

cli/src/utils.rs

    let is_windows = cfg!(windows) && !Paint::enable_windows_ascii();
    let env_colour_disabled = std::env::var("NO_COLOR").is_ok();
-    if is_windows || env_colour_disabled {
+    if is_test || is_windows || env_colour_disabled {


Windows doesn't support color out of the box so instead of replacing every ANSI escape code in our fixtures I just opted to disable colors in tests. I think this is OK, if the colors are off it's not critical and we'd know easily anyway

onbjerg · 2022-07-03T01:23:24Z

cli/test-utils/src/util.rs

 });

 impl OutputExt for process::Output {
    #[track_caller]
    fn stdout_matches_path(&self, expected_path: impl AsRef<Path>) -> &Self {
        let expected = fs::read_to_string(expected_path).unwrap();
-        let expected = IGNORE_IN_FIXTURES.replace_all(&expected, "");
+        let expected = IGNORE_IN_FIXTURES.replace_all(&expected, "").replace('\\', "/");


This is because ArtifactId::identifier does not contain a canonicalized path

onbjerg · 2022-07-03T01:25:01Z

cli/test-utils/src/util.rs

+    format!(
+        "{}={}/",
+        src,
+        dest.trim_end_matches('/')
+            .replace('/', std::str::from_utf8(&[std::path::MAIN_SEPARATOR as u8]).unwrap())
+    )


Remappings from Config on Windows seem to use Windows paths, except if part of the path is from another remapping (see the config tests for examples).

The remappings we use to compare in tests however are not canonicalized. I'm not sure what the best fix is given how flexible remappings are

onbjerg · 2022-07-03T01:25:35Z

cli/tests/it/cmd.rs

-forgetest!(canhandle_direct_imports_into_src, |prj: TestProject, mut cmd: TestCommand| {
+// Tests that direct import paths are handled correctly
+//
+// NOTE(onbjerg): Disabled for Windows -- for some reason solc fails with a bogus error message


I really have NO IDEA at all what is going on here. "Invalid implicit conversion from struct Bar memory to struct Bar memory"????

lol that makes no sense lol
let's keep this ignored for windows

onbjerg · 2022-07-03T01:26:30Z

cli/tests/it/config.rs

-            "nested/=lib/nested-lib/lib/nested/".parse().unwrap(),
+            // NOTE(onbjerg): For some reason a part of this remapping was not normalized on
+            // Windows
+            format!("{}another-lib/src/", remapping_str("another-lib/", "lib/nested-lib/lib/"))


Not sure what's going on here... I think it might be because the path contains other remappings? It's a bit hard to follow

onbjerg · 2022-07-03T01:26:36Z

cli/tests/it/config.rs

-            "nested/=lib/nested-lib/lib/nested/".parse().unwrap(),
+            // NOTE(onbjerg): For some reason a part of this remapping was not normalized on
+            // Windows
+            format!(


Same as above

onbjerg · 2022-07-03T01:26:58Z

fmt/src/formatter.rs

        struct PrettyString(String);

+        impl PartialEq for PrettyString {
+            fn eq(&self, other: &PrettyString) -> bool {
+                self.0.lines().eq(other.0.lines())


Avoids having to replace \r\n with \n

onbjerg · 2022-07-03T02:50:17Z

Finally! I reverted the workflow back to its original state(ish), but a successful run can be found here: https://github.com/foundry-rs/foundry/runs/7165826844 (note: long testing time is cus I accidentally removed the cache step lol)

onbjerg · 2022-07-03T02:51:36Z

cli/test-utils/src/script.rs

        let testdata = format!("{}/../../testdata", env!("CARGO_MANIFEST_DIR"));
-        std::fs::hard_link(
+        std::fs::copy(


Hard links from some non-temp directory to a temp directory is considered a violation of filesystem boundaries on Windows, so we have to copy instead 🙄

Useful for debugging #2092 and #2085

This vastly simplifies fixtures, especially for platforms that do not support color

On Windows hard links cannot cross filesystem boundaries, which temp folders are considered.

mattsse

awesome!

not sure what to do about the remapping_str.
perhaps easiest solution would be using path_slash::PathBufExt to convert all slashes in the remappings?

mattsse · 2022-07-03T14:54:27Z

.cargo/config.toml

+rustflags = [
+  # Increases the stack size to 10MB, which is
+  # in line with Linux (whereas default for Windows is 1MB)
+  "-C", "link-arg=/STACK:10000000",


mattsse · 2022-07-03T14:56:25Z

cli/test-utils/src/util.rs

+///
+/// NOTE: This probably should be unnecessary, and remappings should probably
+/// be canonicalized.
+pub fn remapping_str(src: &str, dest: &str) -> String {


should we use paths-slash crate here instead?

I wonder if we should do that in ethers-solc instead - not sure if using / would break anything internally, but I think at a minimum it would be safe to use / in Display?

that makes sense!

mattsse · 2022-07-03T14:58:14Z

cli/tests/it/cmd.rs

-forgetest!(canhandle_direct_imports_into_src, |prj: TestProject, mut cmd: TestCommand| {
+// Tests that direct import paths are handled correctly
+//
+// NOTE(onbjerg): Disabled for Windows -- for some reason solc fails with a bogus error message


lol that makes no sense lol
let's keep this ignored for windows

gakonst · 2022-07-03T16:27:56Z

High level lgtm. Wonder if we should think about it more or ship it and let Windows users tell us what works well and what doesn't?

onbjerg · 2022-07-03T16:36:11Z

@gakonst I think ship and figure it out, there are probably other bugs lurking but it's hard to find

onbjerg added the L-ignore Log: ignore PR in changelog label Jun 27, 2022

onbjerg force-pushed the onbjerg/windows-integration-tests branch 2 times, most recently from 8aa5c3f to 38e3d6b Compare June 27, 2022 07:55

onbjerg marked this pull request as draft June 27, 2022 07:55

onbjerg force-pushed the onbjerg/windows-integration-tests branch from 38e3d6b to 2c83fd1 Compare June 27, 2022 07:58

onbjerg mentioned this pull request Jun 27, 2022

forge coverage: Win '/' domain separators causes 0% coverage reports #2136

Closed

2 tasks

onbjerg force-pushed the onbjerg/windows-integration-tests branch 6 times, most recently from 89b8184 to 2bb6cac Compare June 28, 2022 15:37

onbjerg force-pushed the onbjerg/windows-integration-tests branch from c1137c6 to 8236acc Compare June 28, 2022 16:13

onbjerg mentioned this pull request Jun 29, 2022

forge script error #2085

Closed

2 tasks

onbjerg commented Jul 3, 2022

View reviewed changes

onbjerg changed the title ~~test: fix windows integration tests~~ test: fix windows Jul 3, 2022

onbjerg changed the title ~~test: fix windows~~ fix: windows Jul 3, 2022

onbjerg added T-bug Type: bug and removed L-ignore Log: ignore PR in changelog labels Jul 3, 2022

onbjerg marked this pull request as ready for review July 3, 2022 02:50

onbjerg commented Jul 3, 2022

View reviewed changes

onbjerg added 2 commits July 3, 2022 15:19

test: improve multi_runner asserts a bit

087bd6e

test: temporarily only run windows x-platform test

556010b

onbjerg and others added 15 commits July 3, 2022 15:19

test: add forge script <path>:<name> test

393a949

Useful for debugging #2092 and #2085

test: fix path seps

3bf568e

test: fix ffi on windows

edf025b

fix: abstract contract test

d7c13dd

test: fix fmt tests

7c0ec9a

test: fix line endings

6a05393

fix: canonicalize source path

6c9223f

chore: improve error msg in forge script

9592b86

fix: fix forge script on windows

d1ff88f

test: disable colors in tests

6fbc508

This vastly simplifies fixtures, especially for platforms that do not support color

test: fix remaining tests on windows

f1a1963

build: set windows stack size to 10mb

aa89a52

test: copy script testdata

fa576b5

On Windows hard links cannot cross filesystem boundaries, which temp folders are considered.

ci: restore workflows

a386ec6

chore: lint

ca7b393

onbjerg force-pushed the onbjerg/windows-integration-tests branch from 5912625 to ca7b393 Compare July 3, 2022 13:20

onbjerg added 2 commits July 3, 2022 16:00

build: static msvc C runtime

ed4c6dd

test: disable colors in tests

bcdfc87

onbjerg force-pushed the onbjerg/windows-integration-tests branch from fcfa280 to bcdfc87 Compare July 3, 2022 14:35

mattsse requested changes Jul 3, 2022

View reviewed changes

gakonst merged commit 8e7753d into master Jul 4, 2022

gakonst deleted the onbjerg/windows-integration-tests branch July 4, 2022 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: windows #2132

fix: windows #2132

onbjerg commented Jun 27, 2022 •

edited

onbjerg commented Jun 27, 2022

mattsse commented Jun 27, 2022

onbjerg commented Jun 27, 2022

mattsse commented Jun 27, 2022

onbjerg commented Jun 27, 2022

onbjerg commented Jun 28, 2022 •

edited

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

mattsse Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

mattsse Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg Jul 3, 2022

onbjerg commented Jul 3, 2022 •

edited

onbjerg Jul 3, 2022

mattsse left a comment

mattsse Jul 3, 2022

mattsse Jul 3, 2022

onbjerg Jul 3, 2022

mattsse Jul 4, 2022

mattsse Jul 3, 2022

gakonst commented Jul 3, 2022

onbjerg commented Jul 3, 2022

fix: windows #2132

fix: windows #2132

Conversation

onbjerg commented Jun 27, 2022 • edited

onbjerg commented Jun 27, 2022

mattsse commented Jun 27, 2022

onbjerg commented Jun 27, 2022

mattsse commented Jun 27, 2022

onbjerg commented Jun 27, 2022

onbjerg commented Jun 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onbjerg commented Jul 3, 2022 • edited

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst commented Jul 3, 2022

onbjerg commented Jul 3, 2022

onbjerg commented Jun 27, 2022 •

edited

onbjerg commented Jun 28, 2022 •

edited

onbjerg commented Jul 3, 2022 •

edited