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

One Script to Rule Them All #1810

Closed
wants to merge 1 commit into from

Conversation

danforbes
Copy link

TODO: SGX

@j16r
Copy link
Contributor

j16r commented Oct 25, 2019

Elsewhere in this makefile, you'll see that targets have a trailing comment, so that the command appears in the help output. Give it a shot: make help it would nice to add similar if brief help comments for these new targets.

@danforbes
Copy link
Author

@j16r - yes thank you for the tip! I was trying to figure out how to make that happen but wasn't having much luck 😣 I was relying on code review for just this sort of helpful information 🚀 Please let me know if you have any other suggestions because I'm far from a make wizard 🙏

@danforbes
Copy link
Author

As suggested by @j16r, I have made changes to this PR so that make help continues to display all available commands.

@HenryNguyen5
Copy link
Collaborator

I'm not sure how much we need this for yarn scripts, seems like it would get out of date really fast and be hard to maintain unless this was code-genned. This also doesnt take advantage of yarn workspace commands, nor yarn workspaces commands. See here where we kind of have "one script to rule them all" where those commands will run across the entirety of workspaces.

Also, some of those commands in the makefile (such as build) are system dependent, and instead have a script that handles those issues correctly (such as setup). Other commands have pre-post commands, which should not be included since they should not be run manually by the user.

@danforbes
Copy link
Author

Yes, I wasn't quite sure what the desired outcome of this task was so I kind of just went for it 😅 Certainly happy to think about how this can be improved in any ways. For what it's worth, much of this was code-generated, and I certainly agree with continuing to move things in that direction.

@HenryNguyen5
Copy link
Collaborator

I think what we were aiming for was to have cross-package scripts that were the same script per package, which was partially implemented so far in here, we just havent added them all yet. @se3000 @rupurt might have a clearer answer here

@danforbes
Copy link
Author

@HenryNguyen5, et al. - I am totally open to taking this in whatever direction it needs to go. If a static commit like this isn't the right answer (and I'm inclined to believe it probably isn't), let's figure out a programmatic solution to get us where we need to be. I don't have the context to figure out what that solution should look like but I'll certainly be happy to try to build it if given a spec.

@rupurt
Copy link
Contributor

rupurt commented Oct 27, 2019

I was envisioning something the following make CLI API:

# Install the dependencies of every app in the monorepo e.g. `go get -u`, `yarn install`
make install

# Run tasks across every app to ensure any of them can run e.g. `yarn setup`
make setup

# Update the project after a git pull e.g. `yarn setup`
make update

# Run the main development command across every app.
# We don't have a yarn dev script in the root package.json.
# I feel like we should add one which would run commands like `yarn workspace @chainlink/explorer run dev`
#
# This should also run `$ chainlink`, etc... Perhaps `$ devnet`??
make dev

# Run the main production command across every app command e.g. explorer `yarn workspace @chainlink/explorer run prod`
make prod

# Run the tests across every app e.g. `yarn test`, `go test ./... -p 1`
make test

# Run the linters across every app e.g. `yarn lint`
make lint

# Run any tasks to ensure the app is ready for make test in the CI environment
make cibuild

We've been abstracting the commands required in the node yarn/npm part of the app in package.json scripts as @HenryNguyen5 mentioned. These should handle Typescript, truffle etc... If there's something you need added/updated you can let me know.

@se3000 @j16r do you have any go commands you'd like to see that I've missed?

@danforbes danforbes closed this Nov 7, 2019
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.

None yet

4 participants