Start Refactor tool init specs #4915

bew · 2017-09-01T23:14:22Z

This is a refactor of the crystal tool init specs, it is partly extracted & reworked from #4691.

It fixes 2 main issues:

Use a handy stderr_of(command) helper instead of relying on obscure bash trick like (echo "foo" 1>&2) 2>&1 >/dev/null to get only stderr on stdout...
Provides 3 helpers to create/delete temporary directory/file, and allow to make tool tests in a directory, then delete it (on spec success or failure). Previously there were a lot of directories (created by the tool) not deleted in case of errors in the specs. (and sometime the reported errors weren't coherent between multiple runs)

The 3 temporary-stuff helpers will probably be replaced when #4096 or similar is merged.

asterite · 2017-09-02T02:36:04Z

The init specs are wrong already, they shouldn't invoke the compiler, they should use init programmatically. I wish someone would refractor that instead.

bcardiff

Why on some bin_crystal invocation an expand_path is needed but not in others? And previously there was mos expand_path.

Other than that lgtm

bcardiff · 2017-09-02T16:49:26Z

What asterite said is right, yet the readability improvement is good even init is not called programatically.

bew · 2017-09-02T19:17:48Z

Why on some bin_crystal invocation an expand_path is needed but not in others?

Little bit of context: Previously the commands created some test projects in the compiler specs dir, and in case of failure they just stayed there.

To answer the question: Because I created a temporary directory and cd into it, thus bin/crystal is not available in this directory, so I need to keep his full path somewhere to invoke it!

The init specs are wrong already, they shouldn't invoke the compiler, they should use init programmatically.

I agree with you, I'm trying to do that, but I'm hitting a wall: The errors that was checked by the last specs (on Init) are errors written directly to STDERR, thoses are args handling errors... How should I go about this?

I refactored my PR to reduce the number of helpers and simplify specs.

I think we should merge this refactor as is (unless you have suggestions of course). And when we change how error handling is done for Init (so we can spec it), we'll be able to continue the refactor and spec Init programatically.

bew · 2017-10-24T22:37:18Z

@bcardiff and/or others, can you review this please?

straight-shoota · 2017-11-17T19:52:56Z

LGTM. There is certainly room for further improvements, but this is the first step!

bcardiff · 2017-11-18T14:02:11Z

spec/compiler/crystal/tools/init_spec.cr

+# Creates a temporary directory, cd to it and run the block inside it.
+# The directory and its content is deleted when the block return.
+private def within_temporary_directory
+  tmp_path = "/tmp/crystal_init_spec_tmp_dir-#{rand(10_000)}"


Could #{__DIR__}/tmp... be used instead of /tmp...? The local tmp is already in the .gitignore and I would rather not write something outside the working copy.

Other that that, please rebase and I would 👍

Aah ok, I didn't thought about that, I do that soon!

bew · 2017-11-21T23:18:22Z

@bcardiff done!

A commits squash would reduce the PR complexity, but it's still useful for review. I suppose you can do that when merging so I'll leave it like that ;)

luislavena

Thank you @bew for the PR.

Please take a look to some of the comments related to leftovers and file locations.

Thank you.

luislavena · 2017-11-22T17:21:09Z

spec/compiler/crystal/tools/init_spec.cr

+require "spec"
+require "yaml"
+
+BIN_CRYSTAL = File.expand_path("bin/crystal")


This assumes Dir.current is Crystal's root directory, which might not be the case. I recommend adjust it so bin/crystal can be calculated relatively to this file/directory location instead.

$ which ccr /home/luis/bin/ccr $ file `which ccr` /home/luis/bin/ccr: symbolic link to /home/luis/code/crystal-lang/crystal/bin/crystal $ cd spec/compiler/crystal/tools $ ccr eval 'p Dir.current, File.expand_path("bin/crystal")' Using compiled compiler at `.build/crystal' "/mnt/c/Users/Luis/Code/crystal-lang/crystal/spec/compiler/crystal/tools" "/mnt/c/Users/Luis/Code/crystal-lang/crystal/spec/compiler/crystal/tools/bin/crystal"

True, but it didn't work before too

Regarding specs, it should be reasonable to assume they are run from the project root folder. I don't think it is worth fixing this unless there is a specific issue why this wouldn't work.

@luislavena ah ok I see what you mean, I fixed it!

luislavena · 2017-11-22T17:24:04Z

spec/compiler/crystal/tools/init_spec.cr

+# Creates a temporary directory, cd to it and run the block inside it.
+# The directory and its content is deleted when the block return.
+private def within_temporary_directory
+  tmp_path = "#{__DIR__}/crystal_init_spec_tmp_dir-#{rand(10_000)}"


Now temporary directories will be created relative to the location of each spec file (__DIR__).

In that case, will be great to ignore crystal_init_spec_tmp_dir-* in .gitignore to avoid a broken spec allow possible left over files be available to commit into the codebase.

It's a helper for this spec file only, and unless this method within_temporary_directory crash, the ensure will automatically clean the tmp dir, so I'm not sure it's necessary

The tmp dir should be inside de project root. I think @bcardiff's comment asking for this was a bit imprecise.
The root folder's .gitignore contains /tmp, so you should probably use #{DIR}/../../../../tmp/crystal_init_spec-* as temporary directories.

@straight-shoota I don't think that's what he meant

Currently all these specs output to /tmp, which is in .gitignore. The change here using __DIR__ place these temporary files relative to the spec file, inside the spec folder, which are not ignored.

It's a helper for this spec file only, and unless this method within_temporary_directory crash, the ensure will automatically clean the tmp dir, so I'm not sure it's necessary

That is something we don't know. Perhaps Process.new might segfault, or other part of the spec could, leaving leftovers that were never cleaned up.

I'm not suggesting radically change and refactor what you have done, but instead consider where those temporary files are stored and either A: add them to the gitignore file to avoid accidental commit, or B: adjust the location to be the one relative to the already ignored one.

Thank you.

bew · 2017-11-23T15:07:17Z

Followup of @luislavena comment:

Currently all these specs output to /tmp, which is in .gitignore.

Not sure about that: the list of spec files that uses /tmp is:

spec/std/file_utils_spec.cr
spec/std/socket_spec.cr
spec/std/file_spec.cr
spec/std/dir_spec.cr

So these specs uses the /tmp directory in the root filesystem as a temporary directory.
It is not the directory in .gitignore (which is the one relative to the project's root directory, for me it would be something like: /home/lesell_b/Projects/opensource/crystal/tmp).

I'm not sure the entry /tmp in .gitignore is actually useful for that matter.

According to @bcardiff:

I would rather not write something outside the working copy.

So all these specs 'break' this rule, and uses a directory outside the working copy of the repository.

Back to this PR, I think I'll just use /tmp for now, so this PR can get merged, and I (or someone else) can open another PR to 'fix' the temporary directory used in specs.

WDYT?

luislavena · 2017-11-23T15:22:30Z

Not sure about that: the list of spec files that uses /tmp is:

Currently the specs of init_spec.cr, I'm not evaluating anything outside the context of this PR.

This PR modifies that behavior and I'm commenting on that behavior change. I'm not suggesting go change everything outside this PR, but instead honor the existing behavior or at minimum update the .gitignore pattern so any potential leftovers are correctly excluded.

I'm not sure the entry /tmp in .gitignore is actually useful for that matter.

It was prior this change, all the tmp/ references in init_spec.cr pointed to the relative to current directory, which was the base of Crystal root clone. run_init_project placed files in tmp/ as base and those were ignored by .gitignore pattern.

Please note that all my comments and observations have been around this PR changes. Apologies if you thought my comments requested you introduce changes outside the context of this PR.

Cheers.

bew · 2017-11-23T15:49:45Z

It was prior this change, all the tmp/ references in init_spec.cr pointed to the relative to current directory, which was the base of Crystal root clone. run_init_project placed files in tmp/ as base and those were ignored by .gitignore pattern.

Aaaah thanks I didn't remember that specificity, so I assumed you were talking about a broader issue, sorry about that

I made a change, let me know what you think!

straight-shoota · 2017-11-23T16:43:29Z

Why not just simply use tmp directory in project root? This should be the place where all temporary files should be put inside the working tree. There is no need for additional .gitignore files and paths

bew · 2017-11-23T16:47:46Z

Hmm It's probably just me then, but I don't like putting things outside the spec/ directory, or even outside the directory of the spec file..
I'll put it in the project's tmp dir and we'll discuss more later if necessary.

bew · 2017-11-23T16:52:27Z

Should be good now 😃

luislavena

~~While I would personally prefer to continue to use tmp/~~, the changes I suggested were addressed, so 👍 from me.

RX14 · 2017-11-23T17:52:29Z

We should just use the system's tempdir everywhere, if it's not writable and usable the system is broken anyway. Plus it's way faster because tmpfs is in-memory.

But, that's a topic for another PR and for now we should keep it the same as before. I'll review this in just a bit.

luislavena · 2017-11-24T14:11:05Z

@bew it appears CI failed due formatting error. Can you reformat code using local version of the compiler and push again?

Thank you.

bew · 2017-11-24T17:29:11Z

Thanks @luislavena, fixed!

bew · 2017-12-21T15:04:03Z

Is there something I can do here? Or do we just wait for another review?

bew · 2018-01-03T12:42:16Z

@bcardiff @asterite a second review please?

asterite

I guess this is fine, but I'd really like not to execute bin/crystal inside compiler specs. This should be tested programmatically (could be done in another PR, though)

RX14 · 2018-01-03T13:58:41Z

Very interesting backtrace from travis:

Invalid memory access (signal 11) at address 0x38
[0xd252b5] *CallStack::print_backtrace:Int32 +117
[0x9469ab] __crystal_sigfault_handler +75
[0x1e5d877] sigfault_handler +40
[0x7f7592622390] ???
[0x93b45a] ~procProc(Nil) +42
[0x93b5a3] ~procProc(Nil) +275
[0x1e521cb] GC_mark_some +299
[0x1e4a10d] GC_stopped_mark +125
[0x1e4a88a] GC_try_to_collect_inner +234
[0x1e4b540] GC_collect_or_expand +384
[0x1e4b756] GC_allocobj +246
[0x1e4ea09] GC_generic_malloc_inner +233
[0x1e4eb38] GC_generic_malloc +104
[0x1e4ef3f] GC_core_malloc +239
[0xd5fae6] *GC::malloc<UInt64>:Pointer(Void) +6
[0x93019e] __crystal_malloc +14
[0xd84f85] *Thread::new:Thread +21
[0x93af06] ~Thread::current:init +6
[0x93aef4] ~Thread::current:read +68
[0xd8537e] *Thread#start:(Exception+ | Nil) +14
[0xb72006] ~procProc(Pointer(Void), Pointer(Void)) +6
[0x1e5d7ef] GC_inner_start_routine +79
[0x1e54475] GC_call_with_stack_base +21
[0x7f75926186ba] ???
[0x7f75909653dd] clone +109
[0x0] ???

not sure what to make of it.

RX14 · 2018-01-03T14:04:50Z

Actually given the Thread::current:init in the stacktrace, this seems like a race condition on startup.

I'll merge when CI comes back green.

bew · 2018-01-03T14:17:33Z

@RX14 can you restart the Travis build to see if it happens again?

asterite · 2018-01-03T14:21:02Z

When parallelism is introduced, we'll need to add locks to every class variable and constant access, to make sure it's not initialized twice by two different threads. I wonder why that crash happened given that we don't have multiple threads right now.

bcardiff reviewed Sep 2, 2017

View reviewed changes

bew force-pushed the refactor-init-spec branch from b557729 to fc4386f Compare September 2, 2017 19:20

bew force-pushed the refactor-init-spec branch from 9a1ac56 to 0e4eb81 Compare October 24, 2017 22:31

bew changed the title ~~Refactor tool init specs~~ Start Refactor tool init specs Oct 24, 2017

straight-shoota approved these changes Nov 17, 2017

View reviewed changes

straight-shoota mentioned this pull request Nov 17, 2017

Allow to init a crystal app/lib in an empty directory #4691

Merged

bcardiff reviewed Nov 18, 2017

View reviewed changes

bew force-pushed the refactor-init-spec branch from 0e4eb81 to 6d9c0bc Compare November 21, 2017 20:48

luislavena suggested changes Nov 22, 2017

View reviewed changes

bew force-pushed the refactor-init-spec branch from 6fcb86f to 3333648 Compare November 23, 2017 16:52

luislavena approved these changes Nov 23, 2017

View reviewed changes

RX14 approved these changes Nov 23, 2017

View reviewed changes

bew force-pushed the refactor-init-spec branch from 6035c39 to 7c1cd99 Compare November 28, 2017 01:24

RX14 approved these changes Dec 21, 2017

View reviewed changes

asterite approved these changes Jan 3, 2018

View reviewed changes

RX14 added kind:refactor topic:stdlib:specs topic:stdlib topic:tools topic:compiler and removed topic:stdlib labels Jan 3, 2018

RX14 added this to the Next milestone Jan 3, 2018

Refactor init spec

0115d6d

bew force-pushed the refactor-init-spec branch from 7c1cd99 to 0115d6d Compare January 3, 2018 14:43

RX14 merged commit 288af4b into crystal-lang:master Jan 3, 2018

bew deleted the refactor-init-spec branch January 3, 2018 17:50

lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018

Refactor init spec (crystal-lang#4915)

d373305

Start Refactor tool init specs #4915

Start Refactor tool init specs #4915

Conversation

bew commented Sep 1, 2017 • edited Loading

asterite commented Sep 2, 2017

bcardiff left a comment

Choose a reason for hiding this comment

bcardiff commented Sep 2, 2017

bew commented Sep 2, 2017 • edited Loading

bew commented Oct 24, 2017

straight-shoota commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bew commented Nov 21, 2017 • edited Loading

luislavena 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luislavena Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

bew commented Nov 23, 2017

luislavena commented Nov 23, 2017 • edited Loading

bew commented Nov 23, 2017

straight-shoota commented Nov 23, 2017

bew commented Nov 23, 2017 • edited Loading

bew commented Nov 23, 2017

luislavena left a comment • edited Loading

Choose a reason for hiding this comment

RX14 commented Nov 23, 2017

luislavena commented Nov 24, 2017

bew commented Nov 24, 2017

bew commented Dec 21, 2017

bew commented Jan 3, 2018

asterite left a comment

Choose a reason for hiding this comment

RX14 commented Jan 3, 2018 • edited Loading

RX14 commented Jan 3, 2018 • edited Loading

bew commented Jan 3, 2018

asterite commented Jan 3, 2018

bew commented Sep 1, 2017 •

edited

Loading

bew commented Sep 2, 2017 •

edited

Loading

bew commented Nov 21, 2017 •

edited

Loading

luislavena Nov 23, 2017 •

edited

Loading

luislavena commented Nov 23, 2017 •

edited

Loading

bew commented Nov 23, 2017 •

edited

Loading

luislavena left a comment •

edited

Loading

RX14 commented Jan 3, 2018 •

edited

Loading

RX14 commented Jan 3, 2018 •

edited

Loading