Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start Refactor tool init specs #4915

Merged
merged 1 commit into from Jan 3, 2018
Merged

Conversation

bew
Copy link
Contributor

@bew bew commented Sep 1, 2017

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
Copy link
Member

asterite commented Sep 2, 2017

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

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

bcardiff commented Sep 2, 2017

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

@bew
Copy link
Contributor Author

bew commented Sep 2, 2017

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 bew changed the title Refactor tool init specs Start Refactor tool init specs Oct 24, 2017
@bew
Copy link
Contributor Author

bew commented Oct 24, 2017

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

@straight-shoota
Copy link
Member

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

# 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)}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bew
Copy link
Contributor Author

bew commented Nov 21, 2017

@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 ;)

Copy link
Contributor

@luislavena luislavena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bew for the PR.

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

Thank you.

require "spec"
require "yaml"

BIN_CRYSTAL = File.expand_path("bin/crystal")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but it didn't work before too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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)}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

bew commented Nov 23, 2017

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
Copy link
Contributor

luislavena commented Nov 23, 2017

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
Copy link
Contributor Author

bew commented Nov 23, 2017

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
Copy link
Member

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
Copy link
Contributor Author

bew commented Nov 23, 2017

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
Copy link
Contributor Author

bew commented Nov 23, 2017

Should be good now 😃

Copy link
Contributor

@luislavena luislavena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RX14
Copy link
Contributor

RX14 commented Nov 23, 2017

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
Copy link
Contributor

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

Thank you.

@bew
Copy link
Contributor Author

bew commented Nov 24, 2017

Thanks @luislavena, fixed!

@bew
Copy link
Contributor Author

bew commented Dec 21, 2017

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

@bew
Copy link
Contributor Author

bew commented Jan 3, 2018

@bcardiff @asterite a second review please?

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

RX14 commented Jan 3, 2018

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
Copy link
Contributor

RX14 commented Jan 3, 2018

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
Copy link
Contributor Author

bew commented Jan 3, 2018

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

@asterite
Copy link
Member

asterite commented Jan 3, 2018

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.

@RX14 RX14 merged commit 288af4b into crystal-lang:master Jan 3, 2018
@bew 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants