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

Jump to bottom

Makefile: add 'clean_crystal' target #4268

Merged
merged 1 commit into from Jun 2, 2017
Merged

Makefile: add 'clean_crystal' target #4268

merged 1 commit into from Jun 2, 2017

Conversation

makenowjust
Copy link
Contributor

It is useful in compiler development cycle.

$ vim src/...  # edit compiler sources (but incomplete)
$ make crystal # oops! maked incompilable compiler...
$ make crystal # failed because compiler does not work

At such time I don't want to use make clean because it removes libcrystal and llvmext also. I don't want to build C++ files again, so I added clean-crystal target in Makefile. It removes the files built by crystal command. Now, we can use make clean_crystal crystal to recompile crystal after removing compiler itself.

@makenowjust
Makefile: add 'clean_crystal' target
a60b646 
It is useful in compiler development cycle.

    $ vim src/...  # edit compiler sources (but incomplete)
    $ make crystal # oops! maked incompilable compiler...
    $ make crystal # failed because compiler does not work

At such time I don't want to use `make clean` because it removes
`libcrystal` and `llvmext` also. I don't want to build C++ files
again, so I added `clean-crystal` target in Makefile. It removes
the files built by `crystal` command. Now, we can use
`make clean_crystal crystal` to recompile `crystal` after removing
compiler itself.
@bcardiff
Copy link
Member

I've done something like in my working copy with an additional phony rebuild as rebuild: clean_crystal crystal rule so I can do

$ make rebuild # compile deps and compiler
$ make rebuild # compile just the compiler

But I always wonder if it won't be better to clean_crystal as a dependency of crystal.

@RX14
Copy link
Contributor

RX14 commented Apr 11, 2017

Compiling a stage 2 compiler with a compiled compiler is very useful (I did it a lot with #4182). clean_crystal shouldn't be a dependency of crystal.

@makenowjust
Copy link
Contributor Author

@RX14 Yes. I think so too, and so I don't make clean_crystal as dependency of crystal.

@mverzilli
Copy link

mverzilli commented Jun 2, 2017 •
edited

I think this is useful, but I'm a bit worried about naming. Having clean, crystal and clean_crystal which is not exactly the addition of both seems like it will cause some confusion. Can we think of a more distinctive name? Even stage_2 is probably terrible but better than clean_crystal. Or is it just me?

@bcardiff
Copy link
Member

bcardiff commented Jun 2, 2017

@mverzilli the proposed clean_crystal is actually ensuring a stage_1 compiler since it removed the previously built compiler.

What about fresh_crystal?

@makenowjust
Copy link
Contributor Author

I wrote description for clean_crystal: "Clean up crystal built files". It removes not only .build directory but also doc directory, and its name comes from abbreviation of clean_crystal_built_files.

@RX14
Copy link
Contributor

RX14 commented Jun 2, 2017 •
edited

I think clean_crystal is fine and we should merge this PR as-is, but fresh_crystal would be second best. The standard terminology in makefiles is clean though. fresh_crystal would probably mean fresh_crystal: clean_crystal crystal to me.

@mverzilli mverzilli merged commit cc16950 into crystal-lang:master Jun 2, 2017
@mverzilli mverzilli added this to the Next milestone Jun 2, 2017
@mverzilli
Copy link

Fair enough, I'm probably the least experienced with make in this discussion. If this makes sense to all of you, I have no option but trust you :).

@makenowjust makenowjust deleted the feature/makefile/clean-crystal branch June 2, 2017 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:feature topic:infrastructure
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@makenowjust @bcardiff @RX14 @mverzilli