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

[WIP] Add some love to Makefile #2099

Closed
wants to merge 3 commits into from

Conversation

divan
Copy link
Contributor

@divan divan commented Aug 1, 2020

Motivation

I was familiarizing myself with Makefile and realized that there is no easy way to learn the list of targets and make sense of what they're for. As the love to ASCII art is hard not to notice on Spacemesh website, I thought it would be a good idea to add some ASCII-based help/usage command to Makefile.

Changes

  • new Makefile help is added, which displays fancy usage info and list of existing targets
  • each target mentioned in help now has a comment in the form "## Description" or "##@category Description"

Demo

Screenshot 2020-08-01 at 22 08 53

TODO

  • Make sure if all "public" targets are listed and described correctly
  • Make sure "internal" targets (not intended to be run by developers) are hidden from the help output to keep it concise
  • Verify if it looks acceptable on white background
  • Test on Windows (if there any devs on Windows)
  • Remove placeholder for Spacemesh logo (or find a way to display logo in ASCII)

@y0sher
Copy link
Contributor

y0sher commented Aug 5, 2020

Hi @divan ! this looks awesome. thanks for the PR!
can't wait to see the final result 🤩

@divan
Copy link
Contributor Author

divan commented Aug 7, 2020

Thanks @y0sher. I think first two items in the checklist is more of a question to the core dev team members. I inferred the descriptions and actually used targets just by reading Makefile, and not sure everything is correct. So inputs/feedback would be appreciated here.

@lrettig
Copy link
Member

lrettig commented Sep 17, 2020

This is great, thanks for the contribution @divan! I added some ascii art and slightly changed the way the printing happens in the makefile.

Re: the outstanding tasks here:

Verify if it looks acceptable on white background

It looks okay, not great. You can't read the "spacemesh" text in the box at the top, but all of the other text is visible. I don't think there's much we can do about this. It's still perfectly usable.

Test on Windows (if there any devs on Windows)

It doesn't currently work on Windows, I tested this. It has to do with how the shell interprets this line:

https://github.com/divan/go-spacemesh/blob/140bc569a1a1b76e6cf1cc0ed80b84b887d600dc/Makefile#L80

I don't know how to "export" a make variable to a shell env var in Windows. I tried some of the other ideas in this SO thread but none of them work on Windows, the interpolation rules seem to be different and it doesn't seem to like multi-line env vars.

@lrettig
Copy link
Member

lrettig commented Sep 17, 2020

Since this is purely aesthetic, I think it can be merged without Windows support for now - @divan already wrapped this in if statements to disable it on Windows.

@lrettig lrettig self-requested a review September 17, 2020 19:46
@antonlerner
Copy link
Contributor

closed due to inactivity

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