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

Add Makefile and refactor scripts to use it. #186

Closed
wants to merge 1 commit into from

Conversation

mithro
Copy link
Collaborator

@mithro mithro commented Feb 6, 2020

  • make clean
  • make build
  • make docs
  • make tools
  • make install

Signed-off-by: Tim 'mithro' Ansell me@mith.ro

@googlebot googlebot added the cla: yes All contributors in pull request have signed the CLA with Google. label Feb 6, 2020
@mithro mithro requested a review from hzeller February 6, 2020 18:56
Copy link
Collaborator

@fangism fangism left a comment

Choose a reason for hiding this comment

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

General suggestion:
In shell-scripts, set -e to stop on first error.
#!/bin/bash -e does the same thing.

Makefile Outdated

.PHONY: docs

PREFIX=/usr/local
Copy link
Collaborator

Choose a reason for hiding this comment

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

want to use ?= to let user override? or is this not intended for users?

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

In general, I like having a Makefile as this is the standard 'user interface' for anything buildable (even though we then call into some other build-system).

Some first comments below.

Makefile Outdated
@@ -0,0 +1,72 @@
clean:
rm -rf bazel-*
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not actually remove thigns (the bazel-* things are symbolic links to its internal cache).
But bazel clean does.

Makefile Show resolved Hide resolved
Makefile Outdated

.PHONY: docs

PREFIX=/usr/local
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe make PREFIX?=/usr/local ? That way, it can be overridden by an environment variable.

I'd probably put all these variable definitions at the top of the file.

@true

# Generate man pages from the tools
man/verilog_lint.1: bazel-bin/verilog/tools/lint/verilog_lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be cool if we could describe these just in a single generic rule (man/%.1 : ...), but unfortunately, these tools have /lint/, /syntax/ and /formatter/ in their path, which makes them escape the common pattern matching rules in make. So I guess no dice.


man/verilog_format.1: bazel-bin/verilog/tools/formatter/verilog_format
mkdir -p man
gflags2man --help_flag="--helpfull" --dest_dir man bazel-bin/verilog/tools/formatter/verilog_format
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we submit this, we should then also update the documentation that gflags2man is a requirement for make install

@@ -0,0 +1,165 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this generates docs, I'd rename it go generate-docs.sh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this should probably be a bazel rule in some way?

# ---------------
# Generate the GitHub pages to deploy

TRAVIS_REPO_SLUG=${1:-google/verible}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a confusingly named variable. For the perspective of the user of this script, who has this in their toplevel checked out repo, everything runs locally and nothing has to to with travis.
The travis repo is just the github path ? So this variable should probably be name GITHUB_PATH.

Also looking at all the uses below, it is always assembled together with the full URL, so couldn't we just have

  GITHUB_BASE_URL=https://github.com/google/verible

and use that below ? (but I suspect you want to set something in travis that utilizes this variable in a different way)

@hzeller
Copy link
Collaborator

hzeller commented Feb 12, 2020

Any update ? This would be great to have!

@mithro
Copy link
Collaborator Author

mithro commented Feb 13, 2020

I got distracted by #194

I'm unsure if I will get back to this anytime soon.

@hzeller - I was also wondering if these should actually be bazel rules rather than in the Makefile?

 - `make clean`
 - `make build`
 - `make docs`
 - `make tools`
 - `make install`

Signed-off-by: Tim 'mithro' Ansell <me@mith.ro>
# Generate man pages from the tools
man/verilog_lint.1: bazel-bin/verilog/tools/lint/verilog_lint
mkdir -p man
gflags2man --help_flag="--helpfull" --dest_dir man bazel-bin/verilog/tools/lint/verilog_lint
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hzeller - I think the gfags2man stuff should probably be a bazel rule?

hzeller added a commit that referenced this pull request Feb 21, 2020
This could simplifiy #186

PiperOrigin-RevId: 296482842
@hzeller
Copy link
Collaborator

hzeller commented Feb 21, 2020

There is now a bazel run :install -c opt -- -s /usr/local/bin which allow for native installation.

BUT, unlike a usual 'make ; sudo make install', bazel shall not be run as root, but we choose with the -s option if install shall be invoked with sudo.

This is somewhat different how a Makefile workflow would work...

@mithro
Copy link
Collaborator Author

mithro commented Feb 21, 2020

@hzeller is going to figure out how to make the documentation generation and the man page generation into bazel rules.

This Makefile will then just become a simple tool for those too lazy to remember the bazel commands.

@mithro
Copy link
Collaborator Author

mithro commented Feb 21, 2020

Assigning to @hzeller to take over.

hzeller added a commit that referenced this pull request Feb 24, 2020
Can be used in #186

PiperOrigin-RevId: 296521780
@hzeller
Copy link
Collaborator

hzeller commented Jan 29, 2021

Cleaning out old pull requests. Ideas of this have been implemented in various ways.

@hzeller hzeller closed this Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes All contributors in pull request have signed the CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants