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

Freshen Makefile and update README.md #31

Merged
merged 13 commits into from
Apr 4, 2019
Merged

Freshen Makefile and update README.md #31

merged 13 commits into from
Apr 4, 2019

Conversation

virgilwashere
Copy link
Contributor

@virgilwashere virgilwashere commented Mar 10, 2019

Freshen Makefile and update README.md

README.md

  • Add sudo to revelvant commands
  • Use named languages (bash) in fenced codeblocks
  • Use console in fenced codeblocks for command output

Makefile

install target

  • Use install with permissions set

The permissions on has were 777 from git clone.

  • Add option to use $PREFIX

What if I don't want to install to /usr/local/bin?

make PREFIX=$HOME/.local install now works.

update target

  • Add update target for git fetch
  • Include .PHONY targets

tests

  • Add tests for Makefile changes

  • Uses bats variables for directories

  • ✓ make install creates a valid installation

  • ✓ ..even if has is missing from directory

  • ✓ make update runs git fetch

- [x] Add `sudo` to revelvant commands
- [x] Use named languages (bash) in fenced codeblocks
- [x] Use `console` in fenced codeblocks for command output

- [x] Use `install` with permission mode

The permissions on `has` were 777 from `git clone`.

- [x] Add option to use $PREFIX

What if I don't want to install to /usr/local/bin?

`make PREFIX=$HOME/.local install` now works.

- [x] Add `update` target for `git pull`
- [x] Include .PHONY targets
- [x] Add `sudo` to revelvant commands
- [x] Use named languages (bash) in fenced codeblocks
- [x] Use `console` in fenced codeblocks for command output

- [x] Use `install` with permissions set

The permissions on `has` were 777 from `git clone`.

- [x] Add option to use $PREFIX

What if I don't want to install to /usr/local/bin?

`make PREFIX=$HOME/.local install` now works.

- [x] Add `update` target for `git pull`
- [x] Include .PHONY targets

- [x] Add tests for Makefile changes
- [x] Uses `bats` variables for directories

- ✓ make install creates a valid installation
- ✓ ..even if has is missing from directory
- ✓ make update runs git pull
@kdabir
Copy link
Owner

kdabir commented Mar 11, 2019

Thanks for the PR. To be able to merge it should pass on Travis CI. Could you please check that?

I am away right now but will certainly take a look at it little later in the week.

@virgilwashere
Copy link
Contributor Author

To be able to merge it should pass on Travis CI. Could you please check that?

It was and still is, passing on my local checkout.

I will add some bats' feedback and commit shortly.

@virgilwashere
Copy link
Contributor Author

Well. That was a bit more effort than I expected. But I'm ready to start using bats properly now. (with bats-assert-1 thankyou!)

Copy link
Owner

@kdabir kdabir left a comment

Choose a reason for hiding this comment

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

I would have happily merged it today but the make install target is now broken on macos. any suggestions?

Makefile Outdated Show resolved Hide resolved
@kdabir kdabir merged commit 36e11e2 into kdabir:master Apr 4, 2019
@kdabir
Copy link
Owner

kdabir commented Apr 4, 2019

Sadly, some long opts dont work on macOS. had to change them to short flags :(
also cp -u is also not available. so had to remove it. :(

We can work on those in future commits. Didn't want you to fix them, so committed changes to master. you can review them if that works for you on your setup.

@virgilwashere virgilwashere deleted the makefile-update branch April 4, 2019 21:00
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

2 participants