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.

Sign up for GitHub

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

Misc/makefile improvements #171

Merged
merged 7 commits into from
Mar 13, 2018
Merged

Misc/makefile improvements #171

merged 7 commits into from
Mar 13, 2018

Conversation

Jens0512
Copy link
Member

Makefile now only builds if there as been any changes to the relevant files.
make spec will build bin/spec (if not otherwise specified) and run it; if it were to be run again with no changes to any files made, bin/spec will be reused.
make help also looks much much better.

Jens and others added 5 commits March 13, 2018 09:26
Gitignore sentry-files
dba0def
Uppercased Cli to follow Crystal conventions
2403b3e 
`Cli` => `CLI`
Improved makefile
19cea00 
Makefile now only builds if there as been any changes to the relevant files.
`make spec` will build `bin/spec` (if not otherwise specified) and run it; if it were to be run again with no changes to any files made, `bin/spec` will be reused.
`make help` also looks much much better.
Fixed problem with info, blocking commands being printed.
388a087
@Jens0512
Grammar
44fdd25
faultyserver
faultyserver reviewed Mar 13, 2018
View reviewed changes
Copy link
Member

@faultyserver faultyserver left a comment

Choose a reason for hiding this comment

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

Really nice overall! Left a few comments to address, but other than that I'm ready to merge. Thanks for working on this! :)

Makefile Outdated
check: ## Runs all crystal specs
crystal spec/
# The docs says i should do this
.SUFFIXES: .cr .mt
Copy link
Member

Choose a reason for hiding this comment

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

Take a look at https://stackoverflow.com/a/2011058 for why this is useful. A comment like "ensure that only .cr and .mt files can have suffix rules." would be a bit better, but I honestly don't think it's needed here (we're not converting files between types, so we don't need suffix rules, etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see! So that's what it was for 😅

Makefile Outdated
help: ## Show this help.
@fgrep -h "##" $(MAKEFILE_LIST) | fgrep -v fgrep | sed -e 's/\\$$//' | sed -e 's/##//'
MYST_INTERPRETER_SPEC = spec/all_spec.cr
MYST_IN_LANG_SPEC = spec/myst/spec.mt
Copy link
Member

Choose a reason for hiding this comment

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

spaces please :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh my, will do

Makefile Outdated
INSTALL_LOCATION ?= /usr/local/bin/myst
MYSTBUILD ?= myst

O ?= bin
Copy link
Member

@faultyserver faultyserver Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is O supposed to represent the "object" directory? I'm not a fan of single character names, so I'd like it if this could be expanded to something more descriptive, or at least have a comment about what it represents.

Copy link
Member Author

Choose a reason for hiding this comment

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

O is supposed to represent 'out', i'll just change it to that, this is how crystal's makefile does it, so I just thought they knew best. But, I agree

Makefile Outdated
$(O)/myst: $(SOURCE_FILES)
$(call info_log,Building myst...)
mkdir -p $(O)
crystal build -o $(O)/myst $(MYST_CLI)
Copy link
Member

Choose a reason for hiding this comment

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

I think using shards build for this might be more consistent, rather than relying on $(O) to point to the right place. Not a requirement, just a thought :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that. OUT (which is what i'll change O to) can be set like this make build OUT=.../somewhere, so im pretty sure OUT will be the right place, and $(OUT)/myst is required to be a file for the $(SOURCE_FILES) dependency to work.

Copy link
Member Author

@Jens0512 Jens0512 Mar 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So if you want shards build then just use shards build, not make build is my point. :)

Makefile Outdated
STDLIB_FILES := $(shell find stdlib -type f -name '*.mt')
ALL_FILES := $(SPEC_FILES) $(STDLIB_FILES) $(SOURCE_FILES)

INSTALL_LOCATION ?= /usr/local/bin/myst
Copy link
Member

Choose a reason for hiding this comment

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

Can this default to something like myst-local instead of just myst? I don't want it to conflict with other installations the user might have (e.g., through homebrew).

Copy link
Member Author

Choose a reason for hiding this comment

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

My thoughts was just that we could use this in the actual homebrew installation though (e.g. system "make", "install").

Adressed requested changes
d9254eb 
- O changed to OUT
- Tabs changed to spaces where possible (i.e. not targets)
- Removed .SUFFIXES as it was useless
- Fixed problem that made the .DEFAULT_GOAL example would not work
@Jens0512
Copy link
Member Author

Jens0512 commented Mar 13, 2018 •
edited
Loading

What I am most happy with is actually the help target 😅

Jenss-MacBook-Air:myst:% make help
The main targets are:
  build                      - Builds myst into an executable
  clean                      - Cleans (deletes) docs and executables
  help                       - Show this help
  install                    - Install myst to INSTALL_LOCATION
  myst-spec                  - Runs just the in-language specs
  myst-spec_with_build       - Runs the in-language specs with MYSTBUILD
  spec                       - Runs all specs

Targets defined in local/extra.mk:
  docs                       - Generates the `crystal docs`
  list_all_files             - Lists ALL_FILES
  sentry                     - Starts sentry

The default target is 'spec'

@Jens0512
Copy link
Member Author

Actually don't merge just yet, realized there was no goal of running just the interpreter specs 😅

@Jens0512
Copy link
Member Author

There, now it should be all set for a merge

@faultyserver
Copy link
Member

Awesome! Thank you, and nicely done :)

@faultyserver faultyserver added this to the Next milestone Mar 13, 2018
@faultyserver faultyserver merged commit 270dc5f into myst-lang:master Mar 13, 2018
@Jens0512 Jens0512 deleted the misc/makefile_improvements branch March 13, 2018 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@faultyserver faultyserver faultyserver left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Jens0512 @faultyserver