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

Makefile + proper packaging #117

Closed
wants to merge 2 commits into from
Closed

Conversation

fagg
Copy link

@fagg fagg commented Dec 22, 2021

I do not know whether this is of interest, but thought I would share it anyway.

  1. Adds a third_party directory containing the appropriate headers from stb. Also adds an acknowledgement file (separate README.md) stating that they are licensed under different terms to the base QOI library (either public domain or MIT).
  2. Defines a Makefile to make it easier to build and install the conversion and benchmark tooling.
  3. Adds some basic instructions to README.md.

I can only test this on OpenBSD right now, however I did test it with GNU Make (as well as BSD Make) and it is working fine here. There is no reason that this should not work on Linux or Mac provided my memory serves correctly. I cannot test on Mac, but I can probably test on Linux at some point soon.

I will not be offended if you don't want this - I am doing this mainly to make it easier for myself as I am intending to package this for OpenBSD ports, and just thought I would share. :-)

Makefile Outdated Show resolved Hide resolved
@@ -0,0 +1,36 @@
CFLAGS=-O3 -march=native -mtune=native -Wall -Wextra -pedantic -Ithird_party
Copy link

Choose a reason for hiding this comment

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

Fairly certain -mtune=native is implied when using -march=native but at this point, we're splitting hairs.

https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html

@phoboslab
Copy link
Owner

Thanks for this PR! I believe it's a good idea. I just wanted to give a heads up that I probably wont find the time to test/review it until next year. Likewise with the fixes for OpenBSD.

Thanks for your patience and happy holidays :)

@walksanatora
Copy link

if there is one thing that I need to note it is that after installing libstb-dev on linux mint i was still unnable to compile
any ideas why
(here is the make error)

cc -O3 -march=native -mtune=native -Wall -Wextra -pedantic -Ithird_party `pkg-config --cflags libpng` `pkg-config --libs libpng` -o qoibench qoibench.c
qoibench.c:41:10: fatal error: stb_image.h: No such file or directory
   41 | #include "stb_image.h"
      |          ^~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:8: qoibench] Error 1

CFLAGS=-O3 -march=native -mtune=native -Wall -Wextra -pedantic -Ithird_party
CFLAGS_LIBPNG=`pkg-config --cflags libpng`
LDFLAGS_LIBPNG=`pkg-config --libs libpng`
INSTALL=`which install`
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems pretty redundant/boilerplatey. At least change it to "?="

But i really don't know why anyone would have to change install or rm executables.

$(CC) $(CFLAGS) $(CFLAGS_LIBPNG) $(LDFLAGS_LIBPNG) -o qoibench qoibench.c

qoiconv:
$(CC) $(CFLAGS) $(CFLAGS_LIBPNG) $(LDFLAGS_LIBPNG) -o qoiconv qoiconv.c
Copy link
Contributor

Choose a reason for hiding this comment

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

qoiconv does not use libpng

# CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
# SOFTWARE.

CFLAGS=-O3 -march=native -mtune=native -Wall -Wextra -pedantic -Ithird_party
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use -I/usr/include/stb instead of third_party, see #188 for others saying the same.

@amstan
Copy link
Contributor

amstan commented Apr 8, 2022

BTW, I also wrote a Makefile for the normal qoi tools in this repo: https://github.com/amstan/qoi-fpga/blob/fpga/Makefile

But a few things in my implementation (like splitting compilation of qoiconv and qoibench into a .o file too) only make sense when you're doing the weird stuff I am.

@BenBE
Copy link

BenBE commented Apr 8, 2022

Looking at the Makefile I noticed that it does not quite follow the requirements needed for packaging on Debian/Ubuntu: In particular it lacks support for the ${DESTDIR} variable. Also ${prefix} should be set to /usr/local by default, as is customary following the standards for placement of custom files. For normal installation as part of a distribution like Debian, having a look at the Filesystem Hierarchy Standard does not hurt.

Another feature that'd be nice to have is a precompiled static and dynamic library which other programs can link. The current Makefile installs the header (which also includes the source needed for actual use), but it would be much less surprising if the header actually only contained the pure interface definitions.

While using autotools for this project is likely massive overkill, it would (except for the split of header and implementation) avoid all the above mentioned issues. Using cmake could be another option.

@fagg
Copy link
Author

fagg commented Apr 8, 2022

Somebody else is more than welcome to pick this up, but I frankly have neither the time or interest to continue with this.

@fagg fagg closed this Apr 8, 2022
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.

7 participants