Ignore self-cycles on bsb_helper to suppress false positives #5393

cannorin · 2022-05-26T08:55:39Z

Closes False detection of self cycle error with an explicit module type #5368
Related:
- False positive dependency cycle when a module refers to another of the same name #2249
- ocamldep generates circular dependencies which don't exist ocaml/ocaml#4618

This PR disables the self-cycle checking in bsb_helper, because it is causing many false positives.

Notes:

It seems this was once fixed in False positive dependency cycle when a module refers to another of the same name #2249, but I can confirm this is now happening again.
It seems that build_tests are currently disabled in CI, which contains the original test for False positive dependency cycle when a module refers to another of the same name #2249 but left uncaught for a while.

Signed-off-by: Yuta Sato <[email protected]>

cristianoc · 2022-05-26T09:13:33Z

Aside: build tests are running on Mac GitHub ci.

https://github.com/rescript-lang/rescript-compiler/runs/6607004111?check_suite_focus=true#step:7:8244

cristianoc · 2022-05-26T09:37:50Z

@cannorin one issue is lib/bs/.compiler.log which is used for editor integration.
Before the change it contained:

#Start(1653557791021)
FAILED: Foo has a self cycle
FAILED: Foo2 has a self cycle
#Done(1653557791173)

And after this change it's empty.
That means the users of editor integration won't see that anything is wrong with the build.

cristianoc

See comments about editor integration.

cristianoc · 2022-05-26T09:53:05Z

Since it's currently missing and did not get caught, a test should check what goes into .compiler.log.

cannorin · 2022-05-26T12:31:11Z

@cristianoc

And after this change it's empty.

// Demo2.res
module Foo2 = {}

// Foo2.res
open Demo2
include Foo2

So Foo2 does not contain any self cycle (Foo is the same but written in OCaml syntax).

The point of this PR is to prevent the compiler from failing by falsely detecting it as a self cycle, so I suppose these errors in lib/bs/.compiler.log should in fact go away?

Since it's currently missing and did not get caught, a test should check what goes into .compiler.log.

Yes, but I think this means this test was failing silently, so we didn't notice the problem.
https://github.com/rescript-lang/rescript-compiler/runs/6586458093?check_suite_focus=true#step:7:8206

I checked out master, clean built and installed it, and tried 1) running rescript in jscomp/build_tests/zerocycle and 2) running npx node scripts/ciTest.js -bsb. 1 failed with the self-cycle error, but 2 succeeded without any error messages.

cristianoc · 2022-05-26T12:34:42Z

What I meant is that in case of a real self cycle, after this change, it's not reported in .compiler.log.
So this is one step forward (more correct programs go through) and one step backwards (some legit errors don't show up in the editor anymore).

cristianoc · 2022-05-26T12:35:40Z

I should say that non-self cycles were never reported in .compiler.log so this might be a good opportunity to look into that too.

cannorin · 2022-05-26T12:41:31Z

Then I will add real (self and non-self) cyclic test cases to build_tests, and make sure it gets reported to .compiler.log.

I'm not very sure about how to write a real self-cycle, though. I have a feeling that it would end up in a different kind of compile error such as module not found:

// A.res
include A // module A not found

In contrast, non-self cycle is easy to write:

// A.res
include B

// B.res
include A

cristianoc · 2022-05-26T12:47:28Z

% cat src/A.res 
module B = B

% cat src/B.res 
module A = A

% npx rescript 
rescript: [1/3] src/B.d
FAILED: dependency cycle: src/A.cmj -> src/B.cmj -> src/A.cmj.

% cat lib/bs/.compiler.log 
#Start(1653569073904)
#Done(1653569073915)

I believe it's ninja speaking when it says "dependency cycle". The build system generates a ninja file, and ninja finds the cycle before the compiler ever runs.

cannorin · 2022-05-30T09:53:13Z

It seems there is already a test to check the cycle error: https://github.com/rescript-lang/rescript-compiler/tree/master/jscomp/build_tests/cycle

I'll modify this to check .compiler.log too.

(this should fail) Signed-off-by: Yuta Sato <[email protected]>

cristianoc · 2022-05-30T11:51:10Z

Looks like the test fails where it's expected to fail.

cannorin · 2022-05-30T12:03:32Z

It looks like we should fix ninja/src/build.cc to make sure string* err is logged to FILE* compiler_log.

I'm still trying to figure it out how the code works, so it may take some time.

cristianoc · 2022-05-30T12:08:41Z

It looks like we should fix ninja/src/build.cc to make sure string* err is logged to FILE* compiler_log.

I'm still trying to figure it out how the code works, so it may take some time.

ninja is a separate standalone project which is forked and vendored as a submodule. It should not have direct access to compiler log if possible.
A bit hacky, but would it be possible to have access to ninja's output in case of error, and just report that?

cannorin · 2022-05-30T12:25:46Z

would it be possible to have access to ninja's output in case of error, and just report that?

It should be possible. We can open_process_args_full ninja, wait for it to exit, In_channel.input_all the stderr, and print it if the return code is not 0.

cannorin · 2022-05-31T08:27:53Z

@cristianoc It turned out it is almost impossible without modifying the bundled ninja.

ninja prints error messages to stdout (!?), not stderr.
- We can't just capture the errors without capturing the entire output, which causes a buffered output (e.g. rescript: [1/n] is not animated anymore).
- We could do tee ninja.exe .ninja.log in Unix, but it would be hard to do the same in Windows.
rescript.exe doesn't write to .compiler.log.
- In fact, I can't find anything in the repo that writes to .compiler.log but ninja: https://github.com/rescript-lang/ninja/blob/3a477ee7cac769d25f0a8993612a7ea59411e48c/src/ninja.cc#L1137
- Before this PR, FAILED: Foo has a self cycle appeared in .compiler.log because ninja.exe calls bsb_helper.exe (through build.ninja), which did a wrong self-cycle check.
- rescript.exe is the one who calls ninja.exe, so only rescript.exe can capture ninja's output, but any errors produced by rescript.exe don't go into .compiler.log.
- We could modify rescript.exe to write to .compiler.log in case of cycle error, but I suppose it would be very hacky.

cristianoc · 2022-05-31T10:23:18Z

@cannorin thanks for figuring all that out!
The vendored ninja submodule comes from here: ~~https://github.com/rescript-lang/ninja/tree/g_rescript~~ https://github.com/rescript-lang/ninja/tree/rescript
(in fact, little cleanup of branch names is in order there).

What's the smallest change to ninja that would give us what's needed?

cristianoc · 2022-05-31T10:35:35Z

I've done a little cleanup of branches in the ninja fork. Now the branch is called rescript.
At some point, it could be rebased on master, but that's orthogonal to this issue.

cannorin · 2022-05-31T13:28:59Z

What's the smallest change to ninja that would give us what's needed?

It should be enough to modify https://github.com/rescript-lang/ninja/blob/rescript/src/ninja.cc#L1175-L1179 so that it also writes to .compiler.log.

There is also https://github.com/rescript-lang/ninja/blob/rescript/src/build.cc#L155-L159 who says FAILED, but it seems to be handled in https://github.com/rescript-lang/ninja/blob/rescript/src/build.cc#L195.

fhammerschmidt · 2022-05-31T13:46:31Z

Isn't that basically this PR: https://github.com/rescript-lang/ninja/pull/2/files?

cristianoc · 2022-05-31T13:59:23Z

Ohh indeed it looks like ninja is already writing to compiler log:

    fprintf(compiler_log_,"#Start(%" PRId64 ")\n", GetTimeMillis());

So I guess it just needs to write one more thing there.

Sorry for giving wrong advice.

cristianoc · 2022-06-07T03:17:53Z

@cannorin this is now merged: rescript-lang/ninja#2

I think one needs to update the vendored binaries:

./darwinarm64/ninja.exe
./linux/ninja.exe
./darwin/ninja.exe
./win32/ninja.exe

cannorin · 2022-06-08T09:41:54Z

It looks like ninja/snapshot.js needs to be run on the corresponding OS for each platform (linux, darwin, darwinarm64, win32).

I can only update ./linux/ninja.exe because I only have a Debian (as a development machine; I could use my gaming Win10 PC for win32 if needed).

Can someone handle the win32 and darwin binaries?

cannorin · 2022-06-08T09:44:45Z

(I will revert the above commit if we decide to do this in a separate commit/PR)

cristianoc · 2022-06-08T09:56:46Z

You can take it from CI: which generates them for you:
https://github.com/rescript-lang/ninja/actions/runs/2451973956

That said there was something red on that build, taking a look.

cristianoc · 2022-06-08T09:58:02Z

It was just the arm64 build, which is not yet shipped with the compiler anyway.

cristianoc · 2022-06-08T09:59:04Z

It was just the arm64 build, which is not yet shipped with the compiler anyway.

Taking it back:

git log darwinarm64/ninja.exe
commit 60904abaccc26f58da2b3146831252c7769875d2 (origin/update_ninja_binary, github-desktop-mattdamon108/update_ninja_binary, github-desktop-cannorin/update_ninja_binary, github-desktop-benadamstyles/update_ninja_binary)
Author: ZhangHongbo <[email protected]>
Date:   Wed Jun 2 14:36:05 2021 +0800

    get ninja ready for m1

cristianoc · 2022-06-08T09:59:31Z

OK it has run again and is green now.

Signed-off-by: Yuta Sato <[email protected]>

cannorin · 2022-06-08T10:03:48Z

$ file darwinarm64/ninja.exe
darwinarm64/ninja.exe: Mach-O 64-bit arm64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|BINDS_TO_WEAK|PIE>
$ file darwin/ninja.exe
darwin/ninja.exe: Mach-O 64-bit x86_64 executable, flags:<NOUNDEFS|DYLDLINK|TWOLEVEL|BINDS_TO_WEAK|PIE>
$ file linux/ninja.exe
linux/ninja.exe: ELF 64-bit LSB pie executable, x86-64, version 1 (GNU/Linux), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=21200f70515e3c50b7ae2962f6caf2ebf577ebd5, for GNU/Linux 3.2.0, stripped
$ file win32/ninja.exe
win32/ninja.exe: PE32+ executable (console) x86-64, for MS Windows

👍

Signed-off-by: Yuta Sato <[email protected]>

cannorin · 2022-06-08T10:33:33Z

I added a self-cycle test cycle1, which should error as the following:

rescript: [4/4] src/A-Cycle1.cmj
FAILED: src/A-Cycle1.cmj

  We've found a bug for you!
  /.../rescript-compiler/jscomp/build_tests/cycle1/src/A.res:1:9

  1 │ include A
  2 │
  3 │ let x = 42

  Internal path A-Cycle1 is dangling.
  The compiled interface for module A-Cycle1 was not found.

FAILED: cannot make progress due to previous errors.

Signed-off-by: Yuta Sato <[email protected]>

cannorin · 2022-06-08T10:44:33Z

both cycle and cycle1 now check that rescript build writes the error to both stdout and .compiler.log.
- cycle expects the error: FAILED: dependency cycle: src/a.cmj -> src/b.cmj -> src/a.cmj.
- cycle1 expects the error: FAILED: src/A-Cycle1.cmj ... Internal path A-Cycle1 is dangling.
zerocycle now checks that rescript build doesn't error.

and these tests are now passing on my machine.

@cristianoc can you trigger the CI and see if they also pass in CI?

jscomp/build_tests/zerocycle/src/bar.res

Signed-off-by: Yuta Sato <[email protected]>

cannorin · 2022-06-08T10:53:29Z

https://github.com/rescript-lang/rescript-compiler/runs/6791899301?check_suite_focus=true

Run tests (macOS) (which should test build_tests) doesn't seem to be running?

cannorin · 2022-06-08T11:14:51Z

I think
https://github.com/rescript-lang/rescript-compiler/blob/81029965534da402a708b3ba61024176ad13922e/.github/workflows/ci.yml#L60
is not working as intended?

I suppose macos-latest is just an alias and the actual value of runner.name may be macos-11.
I think we can safely use matrix.os instead.

cristianoc · 2022-06-08T11:20:31Z

I think

https://github.com/rescript-lang/rescript-compiler/blob/81029965534da402a708b3ba61024176ad13922e/.github/workflows/ci.yml#L60

is not working as intended?

I suppose macos-latest is just an alias and the actual value of runner.name may be macos-11.
I think we can safely use matrix.os instead.

Good catch.

cristianoc · 2022-06-08T11:28:05Z

Here: #5420

cannorin · 2022-06-08T11:40:17Z

@cristianoc thank you very much for taking care of this PR! 🙏

cristianoc · 2022-06-08T11:41:03Z

@cannorin thanks for the great work! This was a tricky one to pull off.

zth · 2022-06-08T11:55:45Z

Great work @cannorin ! 👏

Ignore self-cycles

3499809

Signed-off-by: Yuta Sato <[email protected]>

cannorin mentioned this pull request May 26, 2022

False detection of self cycle error with an explicit module type #5368

Closed

cristianoc requested changes May 26, 2022

View reviewed changes

build_tests/cycle: check .compiler.log too

a77da2e

(this should fail) Signed-off-by: Yuta Sato <[email protected]>

cannorin force-pushed the 5368-ignore-self-cycles branch from 9c38815 to a77da2e Compare May 30, 2022 10:05

Update the bundled ninja

b587186

Signed-off-by: Yuta Sato <[email protected]>

cannorin force-pushed the 5368-ignore-self-cycles branch from 41ece46 to b587186 Compare June 8, 2022 10:02

add jscomp/build_tests/cycle1

d874477

Signed-off-by: Yuta Sato <[email protected]>

jscomp/build_tests/zerocycle: assert success

fa82f06

Signed-off-by: Yuta Sato <[email protected]>

cristianoc reviewed Jun 8, 2022

View reviewed changes

jscomp/build_tests/zerocycle/src/bar.res Show resolved Hide resolved

cristianoc approved these changes Jun 8, 2022

View reviewed changes

jscomp/build_tests/zerocycle: link the original issue

1d64828

Signed-off-by: Yuta Sato <[email protected]>

cristianoc merged commit be61180 into rescript-lang:master Jun 8, 2022

cannorin deleted the 5368-ignore-self-cycles branch June 8, 2022 11:28

cristianoc mentioned this pull request Jul 12, 2022

Compiler log unterminated on no-op build. #5545

Closed

Ignore self-cycles on bsb_helper to suppress false positives #5393

Ignore self-cycles on bsb_helper to suppress false positives #5393

Conversation

cannorin commented May 26, 2022 • edited Loading

cristianoc commented May 26, 2022 • edited Loading

cristianoc commented May 26, 2022

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc commented May 26, 2022

cannorin commented May 26, 2022 • edited Loading

cristianoc commented May 26, 2022

cristianoc commented May 26, 2022

cannorin commented May 26, 2022 • edited Loading

cristianoc commented May 26, 2022

cannorin commented May 30, 2022

cristianoc commented May 30, 2022

cannorin commented May 30, 2022 • edited Loading

cristianoc commented May 30, 2022

cannorin commented May 30, 2022

cannorin commented May 31, 2022 • edited Loading

cristianoc commented May 31, 2022 • edited Loading

cristianoc commented May 31, 2022

cannorin commented May 31, 2022

fhammerschmidt commented May 31, 2022

cristianoc commented May 31, 2022 • edited Loading

cristianoc commented Jun 7, 2022 • edited Loading

cannorin commented Jun 8, 2022

cannorin commented Jun 8, 2022 • edited Loading

cristianoc commented Jun 8, 2022

cristianoc commented Jun 8, 2022

cristianoc commented Jun 8, 2022

cristianoc commented Jun 8, 2022

cannorin commented Jun 8, 2022

cannorin commented Jun 8, 2022

cannorin commented Jun 8, 2022

cannorin commented Jun 8, 2022

cannorin commented Jun 8, 2022

cristianoc commented Jun 8, 2022

cristianoc commented Jun 8, 2022

cannorin commented Jun 8, 2022

cristianoc commented Jun 8, 2022

zth commented Jun 8, 2022

cannorin commented May 26, 2022 •

edited

Loading

cristianoc commented May 26, 2022 •

edited

Loading

cannorin commented May 26, 2022 •

edited

Loading

cannorin commented May 26, 2022 •

edited

Loading

cannorin commented May 30, 2022 •

edited

Loading

cannorin commented May 31, 2022 •

edited

Loading

cristianoc commented May 31, 2022 •

edited

Loading

cristianoc commented May 31, 2022 •

edited

Loading

cristianoc commented Jun 7, 2022 •

edited

Loading

cannorin commented Jun 8, 2022 •

edited

Loading