Skip to content

Conversation

@Pike
Copy link
Contributor

@Pike Pike commented May 18, 2020

On macos, GNU make eagerly avoids calling into the shell,
so exposing node commands through $PATH doesn't work half of the
time.

I've put two alternatives up in the PR to give some context.

In #486, you mentioned you'd prefer absolute paths. I've done that in fluent-dedent. I've done it for commands that need it, so tsc and eslint. I've done it consistently for eslint, though only one needs it. I've not done it for rollup or nyc. Looking at my changes, I'm not fond.

I've also thrown out the start of doing $(ESLINT) in fluent-dom. Turns out that's actually much more makeier, now that I think about it.

@stasm, mind if I go for the latter consistently, and also drop the $(ROOT)/node_modules/.bin from $PATH?

@stasm
Copy link
Contributor

stasm commented May 18, 2020

My hope was that with time we could remove common.mk completely. I don't like the fact that the build system lives between three different Makefiles for each package.

My suspicion about the makiness is that it comes from the need/desire to allow toolchain overrides like CC=clang. I don't think we'd need this for eslint or even tsc. But I do see the value of declaring build tools executables upfront at the beginning of the Makefile. Could we do that in package-specific Makefiles rather than in common.mk?

I'll be happy to see export PATH gone completely, if possible :)

Pike added 3 commits May 18, 2020 17:09
On macos, GNU make eagerly avoids calling into the shell,
so exposing node commands through $PATH doesn't work half of the
time.

Also, refactor build system a bit to have less repitition so
that the build is easier to maintain. Or at least I think so.
@Pike Pike marked this pull request as ready for review May 18, 2020 15:49
@Pike Pike requested a review from stasm May 18, 2020 15:49
@Pike
Copy link
Contributor Author

Pike commented May 18, 2020

I never knew that mocha was actually a executable command, and not an option ;-)

Anyway, this passes locally now, and on Travis.

I'm not really sure if mocha-test should also be $(MOCHA_CMD).

Given we talked about changing our docs, I like that they're now built consistently.

In all those factorizations, I left the top-level target untouched, and only called into a shared workflow from there, so that local customizations can be done w/out a global change.

I've split this into a few commits to allow for individual decisions.

@Pike Pike self-assigned this May 18, 2020
Copy link
Contributor

@stasm stasm left a comment

Choose a reason for hiding this comment

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

Thanks, @Pike. This looks great, I really like how all the tools are listed at the beginning of the file.

I have one comment/suggestion about the mocha-test target below, but it's nothing blocking.

common.mk Outdated
# Shared recipes
.PHONY: mocha-test

mocha-test:
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is a target rather than MOCHA_CMD because you wanted to be able to resolve TEST_REQUIRES lazily? If instead we use a custom test target in fluent-dom (see my other suggestion), that problem would go away.

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 target 'cause I done this one first, and then the other ones didn't work as targets.

I'm pretty confident that this would work just as well as a CMD. Which is why I kinda looked at it with an angle when pushing.

I think I should just align that.

@Pike
Copy link
Contributor Author

Pike commented May 19, 2020

I was torn on whether I should call it MOCHA_CMD or NYC_CMD, but in the end I figured we're running mocha tests inside a nyc test runner.

If that's just me and the thing I learned new while writing this patch, I'm happy to change that.

common.mk Outdated

MOCHA_CMD =@$(NYC) --reporter=text --reporter=html $(MOCHA) \
--recursive --ui tdd \
--require esm $(TEST_REQUIRES) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: perhaps call it MOCHA_REQUIRES?

@Pike Pike merged commit 0b9e3d2 into projectfluent:master May 19, 2020
@Pike Pike deleted the macos-make branch May 19, 2020 09:40
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