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

Makefile: setup: fix missing dependencies. #388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

casasnovas
Copy link

The Makefile was using a "Match-Anything Pattern Rule" (see https://www.gnu.org/software/make/manual/html_node/Match_002dAnything-Rules.html) as a hand-wavy way of calling target rules into the caravel repository.

Despite the fact this rule depends on 'check-caravel', there was no actual dependency on the caravel repository being fully cloned. Indeed, when the install rule was called (the one cloning caravel), the directory caravel is created as one of the first step, but actual checkouts of the files within that repository is delayed after full cloning and compression of the repository. As such, any rule dependending on the caravel Makefile could fail.

In order to clean this up a little:

  • The ambiguous rule 'install' was renamed to 'install_caravel'

  • The "Match-Anything Pattern Rule" is removed in favour of a more explicit rule, still using pattern matching, and depending explicitly on install_caravel, enforcing the latter has finished before the former starts.

The Makefile definitely would need a bit of love at this point though... so this is just temporary band-aid in the hope it'd be useful for the next person hitting this problem.

The Makefile was using a "Match-Anything Pattern Rule" (see
https://www.gnu.org/software/make/manual/html_node/Match_002dAnything-Rules.html)
as a hand-wavy way of calling target rules into the caravel repository.

Despite the fact this rule depends on 'check-caravel', there was no actual
dependency on the caravel repository being fully cloned.  Indeed, when the
`install` rule was called (the one cloning caravel), the directory
`caravel` is created as one of the first step, but actual checkouts of the
files within that repository is delayed after full cloning and compression
of the repository.  As such, any rule dependending on the caravel Makefile
could fail.

In order to clean this up a little:

  - The ambiguous rule 'install' was renamed to 'install_caravel'

  - The "Match-Anything Pattern Rule" is removed in favour of a more
  explicit rule, still using pattern matching, and depending explicitly on
  `install_caravel`, enforcing the latter has finished before the former
  starts.

The Makefile definitely would need a bit of love at this point though... so
this is just temporary band-aid in the hope it'd be useful for the next
person hitting this problem.

Signed-off-by: Quentin Casasnovas <[email protected]>
@d-m-bailey
Copy link

@casasnovas Thanks for looking into this and working on a solution.

The intention was for the user to be able to access the targets in the caravel/Makefile without having to explicitly state that the targets were in the caravel/Makefile.

That being said, dependencies are not valid on PHONY statements and PHONY statements cannot handle pattern rules.
So in the next update, I'll be changing

.PHONY: % : check-caravel
%:
        export CARAVEL_ROOT=$(CARAVEL_ROOT) && export MPW_TAG=$(MPW_TAG) && $(MAKE) -f $(CARAVEL_ROOT)/Makefile $@

to

%: check-caravel
        @if [ -f $(CARAVEL_ROOT)/Makefile ]; then\
               echo "Running $@ from \$CARAVEL_ROOT/Makefile..."
               CARAVEL_ROOT=$(CARAVEL_ROOT) MPW_TAG=$(MPW_TAG) $(MAKE) -f $(CARAVEL_ROOT)/Makefile $@;\
        fi

The default caravel directory (empty) exists in the user project repos. If the user intends to use a different location, it will be their responsibility to manually create the directory before running any make commands.

I think I'll change the install target to install_caravel for clarity per your suggestion.

Thanks again for the review. Please let me know if you find anything else or disagree with the above decisions.

Incidentally, the PHONY targets for non files seem to generate an unneeded

make[1]: Nothing to be done for 'Makefile'.

message.

Adding a PHONY target for Makefile eliminates this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants