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

Update makefile #329

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update makefile #329

wants to merge 1 commit into from

Conversation

Hibou57
Copy link

@Hibou57 Hibou57 commented May 7, 2020

Removed DESTDIR which did not appear to be defined, moved ldoc.lua to the ldoc directory (on install only) to avoid encumbering the share directory, added proper retrieval of Lua version to get proper LUA_SHAREDIR, made variables evaluations, immediate (= vs :=), removed redundant creation of the share directory (it’s already there if Lua is properly installed, which is implied). Additionally, the file should be renamed as Makefile.

My apologies if I just missed something and this change proposal is irrelevant.

Removed `DESTDIR` which did not appear to be defined, moved` ldoc.lua` to the `ldoc` directory to avoid encumbering the share directory, added proper retrieval of Lua version to get proper `LUA_SHAREDIR` directory. Additionally, the file should be renamed as `Makefile`.

My apologies if I just missed something and this change proposal is irrelevant.
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute.

Some of this is okay, some is not.

  • DESTDIR needs to come back, that can and often is defined outside of the makefile as part of distro packaging commands, e.g. make install DESTDIR=/tmp/package/path. You can't remove that or most distros won't be able to build packages!
  • The filename is okay, both capitalized and not are valid.
  • You can't assume directories exist. There are many different kinds of installs and not all of them look like you expect. Building packages in a chroot is one place this may be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants