tshort / StaticCompiler.jl

Compiles Julia code to a standalone library (experimental)
Other
494 stars 31 forks source link

Move reusable functions to helpers folder #10

Closed aminya closed 4 years ago

aminya commented 4 years ago

Moved reusable functions to src folder. It makes expanding and developing the package easier.

This may be temporary until we reach a stable API.

codecov-io commented 4 years ago

Codecov Report

Merging #10 into master will decrease coverage by 0.57%. The diff coverage is 91.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   91.66%   91.09%   -0.58%     
==========================================
  Files          12       12              
  Lines         396      449      +53     
==========================================
+ Hits          363      409      +46     
- Misses         33       40       +7
Impacted Files Coverage Δ
src/StaticCompiler.jl 100% <ø> (ø) :arrow_up:
src/ccalls.jl 95.83% <100%> (ø) :arrow_up:
src/extern.jl 100% <100%> (ø) :arrow_up:
src/globals.jl 95.65% <100%> (-2.18%) :arrow_down:
src/irgen.jl 87.39% <91.04%> (+2.54%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ac5dfd7...3bfec97. Read the comment docs.

tshort commented 4 years ago

I'm not a fan of moving temporary stuff into the core code. We'd never want the code you moved in the base part of the package. Eventually, we'll want some compiler helpers, but it won't be this code.

If you're looking to make it easier to test out on different code, I'd prefer a script under test or examples that pulls in code needed.

aminya commented 4 years ago

I'm not a fan of moving temporary stuff into the core code. We'd never want the code you moved in the base part of the package. Eventually, we'll want some compiler helpers, but it won't be this code.

If you're looking to make it easier to test out on different code, I'd prefer a script under test or examples that pulls in code needed.

I can make a separate module (or package) if you want. For now, I will move them to helpers folder.

The reason I find moving the code to src or some package useful is that we clearly see which functions are reusable and so we can work on them (or even use them in other works).

This PR is not just code-moving, I also created some functions that do specific tasks.

tshort commented 4 years ago

I prefer one of the following:

aminya commented 4 years ago

I will move it to root/helpers directory.

aminya commented 4 years ago

Done!

aminya commented 4 years ago

Test passes on Windows.

On Linux, there is a problem with @jlrun https://travis-ci.com/aminya/StaticCompiler.jl/jobs/269406893#L223

tshort commented 4 years ago

On Linux, tests fail for me locally, too.

tshort commented 4 years ago

It's a weird macro error. @jlrun works in the REPL, but inside a @testset, it fails.

tshort commented 4 years ago

@aminya, any idea what changed since the last time you tried this approach?

aminya commented 4 years ago

It's a weird macro error. @jlrun works in the REPL, but inside a @testset, it fails.

Here is the diff from the last commit that the test was passed (https://github.com/tshort/StaticCompiler.jl/pull/10/commits/f9727bb267895a954f8aaa34f8ceb69709886e44 - test)

https://github.com/aminya/StaticCompiler.jl/compare/f9727bb..4cc3e4a

That is very strange.

The only difference is that I calculate shellcmd once as a variable in the package, because it is used in multiple functions. link jlrun uses shellcmd. I don't think this is the reason though.


Also, It might be related to path of the files because @__DIR__ is used in the jlrun macro. https://github.com/aminya/StaticCompiler.jl/blob/4cc3e4a8059c66614c41d3a371f87c68f74109cf/src/helpers/jlrun.jl#L34

aminya commented 4 years ago

I thought it might be because of the old gcc compiler on Linux vs the new one on Windows. I added a Linux Bionic test. However, it is not the case

Also, I added MacOS. It fails on the same code, but with a slightly different error: Linux - 3 of them fail https://travis-ci.com/aminya/StaticCompiler.jl/jobs/269898292#L222 MacOS - 1 of them fails https://travis-ci.com/aminya/StaticCompiler.jl/jobs/269898293#L188

So this part of the code seems to be highly dependent on the OS.

tshort commented 4 years ago

The failing tests are bizarre, especially since you had a previous version that I thought worked.

It's also a sign that @jlrun isn't very robust.

aminya commented 4 years ago

I have created issues for the problems. I think this should be merged for now and fix the issues in other PRs. https://github.com/tshort/StaticCompiler.jl/issues/12 https://github.com/tshort/StaticCompiler.jl/issues/13

tshort commented 4 years ago

I'd rather not revert tests. It'd be better to fix things first.

aminya commented 4 years ago

@tshort so should we merge this the same way you decided in https://github.com/tshort/StaticCompiler.jl/pull/14#issuecomment-570783058

tshort commented 4 years ago

The difference is that these test failures are reproducible. I'll try and see if I can identify the root problem.

tshort commented 4 years ago

When run with @jlrun, the libc error code is changed to 4 (interrupted system call). If it's flipped around, the test passes. This works: @test (@jlrun f1()) == f1().

Based on that, I'm inclined to merge, but I want to poke around more. It might be better to remove those rather than use test_skip.

tshort commented 4 years ago

Let's work on fixes on master. The top priority is fixing Mac tests or removing them.

aminya commented 4 years ago

When run with @jlrun, the libc error code is changed to 4 (interrupted system call). If it's flipped around, the test passes. This works: @test (@jlrun f1()) == f1().

Based on that, I'm inclined to merge, but I want to poke around more. It might be better to remove those rather than use test_skip. I'm glad you found the source of the error!

tshort commented 4 years ago

Well, crossing fingers didn't help. The issue from #14 is popping up on master after this merge. Tests are only intermittently passing.