Make buidler work on windows #439

fvictorio · 2020-02-09T16:32:10Z

This PR has some initial changes to make buidler work on windows.

The scripts/install.sh and scripts/run-tests.sh scripts were rewritten in shelljs.
A GitHub action was added.
cross-env is used to make relative paths in scripts work. But, for this to work, the deletion of .bin directories in the install script was commented out. This makes the whole purpose of using a relative path in the packages scripts unnecessary, so cross-env could just be replaced with the local binaries.

The run-tests.sh scripts inside each package weren't migrated yet.

fvictorio · 2020-02-09T16:43:32Z

This is the last run in actions for this: /~https://github.com/fvictorio/buidler/actions/runs/36616440 which failed because it couldn't find cross-env. A previous run with pretty much the same configuration did pass /~https://github.com/fvictorio/buidler/actions/runs/36588608 Not sure how to fix this.

One approach that I'm pretty sure would work for sure is to have cross-env installed in the root node_modules, and then do this moutful in each script:

"build": "node ../../node_modules/cross-env/src/bin/cross-env.js ../../node_modules/.bin/tsc --build src",

which:

Use cross-env so that environment variables and relative paths work
Run cross-env using the node path/to/bin.js trick recommended in this answer
Use the binary (tsc in this case) installed in the root directory

It goes without saying that this is convoluted and ugly. My guess is that the proper solution is related to fixing the problem with lerna and package binaries, but I have no idea what the problem is and how to fix it.

fvictorio · 2020-02-09T18:04:52Z

My bad, I forgot to add node scripts/install.js to the CI. It was working before because it was in a postinstall, but I'm not sure if it's safe to have it there.

Here's the action in action: /~https://github.com/fvictorio/buidler/runs/434787498

The tests step passes because the shelljs scripts aren't throwing when a command fails. I will fix that next.

The windows runner in GitHub Actions has autocrlf set to true by default. This means that files are checked out with CRLF line endings, which causes the linter to throw a lot of errors. This should fix that.

alcuadrado · 2020-02-12T21:03:49Z

packages/buidler-core/test/internal/core/typescript-support.ts

@@ -59,8 +60,8 @@ describe("Typescript support", function() {
      });

      assert.deepEqual(tests.sort(), [
-        await fsExtra.realpath("test/js-test.js"),
-        await fsExtra.realpath("test/ts-test.ts")
+        await fsExtra.realpath("test/js-test.js").then(slash),


I don't get why these need to be slashed. They are file paths.

TASK_TEST_GET_TEST_FILES returns the files with unix slashes, even in windows. I thought it was easier to change the tests than the functionality.

That's surprising, and will probably lead to bugs in the future.

I think the problem comes from our glob module. I'd rather normalize the paths to use the system's separators before returning them from it. This means modifying glob and globSync.

Yeah, I thought about that but it seemed a little out of scope. I will do it now 👍

I fixed it adding the realpath option to the glob utils. This fixes the problem and the test passes in both linux and windows. The docs have a caveat:

realpath Set to true to call fs.realpath on all of the results. In the case of a symlink that cannot be resolved, the full absolute path to the matched entry is returned (though it will usually be a broken symlink)

But that doesn't seem like it will be an issue here.

alcuadrado · 2020-02-12T21:06:26Z

packages/buidler-core/test/internal/util/scripts-runner.ts

@@ -36,8 +36,12 @@ describe("Scripts runner", function() {
    );
    assert.equal(statusCode1, 0);

-    const statusCode2 = await runScript("./failing-script.js");
-    assert.notEqual(statusCode2, 0);
+    const statusCode2 = await runScript(


Why was this change needed?

I don't remember if the extra arguments were needed. The check for 123 was because at some point this tests was (wrongly) passing when the script failed with a 1 exit code.

Oh, I see. Good catch! Can we leave the 123 and remove the extra params if they aren't necessary?

Done. It passes on my machine, will see if they continue to pass on windows (it would be extremely weird if they don't).

alcuadrado · 2020-02-12T21:10:04Z

scripts/run-tests.js

+
+shell.config.fatal = true // throw if a command fails
+
+const isLinux = os.type() === "Linux"


Can we do isWindows instead? This script is also used during development, and many contributors use mac.

The isLinux variable is only used for ignoring the buidler-vyper tests in windows and mac. In windows, docker is available but defaulting to windows containers and changing it to linux containers is not straightforward (at least I wasn't able to do it easily). In mac, inside github actions, docker is not available for, I think, licensing reasons.

Migrate install and run-tests scripts to shelljs

f56d3a6

fvictorio requested a review from alcuadrado February 9, 2020 16:32

Add CI GitHub action

d926fd8

fvictorio force-pushed the work-in-windows branch from 4e74ebb to d926fd8 Compare February 9, 2020 17:45

fvictorio added 4 commits February 9, 2020 15:24

Add .gitattributes to prevent CRLF endings

d862e0e

The windows runner in GitHub Actions has autocrlf set to true by default. This means that files are checked out with CRLF line endings, which causes the linter to throw a lot of errors. This should fix that.

Show installed dependencies in CI workflow

7e2f8ec

Make shelljs throw if some command fails

b62b95a

Run shelljs install script in Travis

2c4d054

fvictorio force-pushed the work-in-windows branch from 1a5230d to 2c4d054 Compare February 9, 2020 18:41

Make tests pass in windows

2ff3665

fvictorio force-pushed the work-in-windows branch 2 times, most recently from 15bf5b8 to 46b6e95 Compare February 12, 2020 00:33

fvictorio added 9 commits February 11, 2020 21:39

Use node path/to/binary for all dev tools, and add rimraf

27c94f8

Migrate all run-tests.sh scripts to shelljs

3a3875b

Enable full matrix in CI

baac1a1

Use unix-style slashes in globalName when resolving an import

9141664

Add test for relative import inside library

51ad0c7

Fix run-tests.js error handling

67fc51e

Only run vyper tests in linux

4f6245e

Run linter only on linux

d63abe5

Fix typo in .gitattributes

ecb3823

fvictorio force-pushed the work-in-windows branch from 46b6e95 to ecb3823 Compare February 12, 2020 00:42

alcuadrado reviewed Feb 12, 2020

View reviewed changes

Don't run buidler-solpp tests in windows

cbc0cde

alcuadrado reviewed Feb 12, 2020

View reviewed changes

fvictorio added 2 commits February 12, 2020 18:35

Increase timeout for scripts-runner test

26b21b6

Remove unnecessary parameters in scripts-runner test

0a0acd6

fvictorio force-pushed the work-in-windows branch from ba2784b to 0a0acd6 Compare February 12, 2020 21:38

alcuadrado changed the base branch from master to windows-support February 12, 2020 21:39

fvictorio added 2 commits February 12, 2020 18:47

Use realpath in glob helpers

9984472

Ignore scripts-runner test on windows

1110dc8

fvictorio marked this pull request as ready for review February 12, 2020 22:18

alcuadrado merged commit c6afc31 into NomicFoundation:windows-support Feb 12, 2020

alcuadrado mentioned this pull request Feb 12, 2020

Windows support and move the CI builds to Github Actions #440

Merged

github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make buidler work on windows #439

Make buidler work on windows #439

fvictorio commented Feb 9, 2020

fvictorio commented Feb 9, 2020

fvictorio commented Feb 9, 2020

alcuadrado Feb 12, 2020 •

edited

Loading

fvictorio Feb 12, 2020

alcuadrado Feb 12, 2020

fvictorio Feb 12, 2020

fvictorio Feb 12, 2020

alcuadrado Feb 12, 2020

fvictorio Feb 12, 2020

alcuadrado Feb 12, 2020

fvictorio Feb 12, 2020

alcuadrado Feb 12, 2020

fvictorio Feb 12, 2020


		shell.config.fatal = true // throw if a command fails

		const isLinux = os.type() === "Linux"

Make buidler work on windows #439

Make buidler work on windows #439

Conversation

fvictorio commented Feb 9, 2020

fvictorio commented Feb 9, 2020

fvictorio commented Feb 9, 2020

alcuadrado Feb 12, 2020 • edited Loading

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

alcuadrado Feb 12, 2020 •

edited

Loading