Skip to content

Conversation

@0x09
Copy link

@0x09 0x09 commented Jan 25, 2026

Hi, thanks for the awesome library.

This adds a make uninstall target that can be used to uninstall the libraries, utf8proc header, and pkg-config file.

An uninstall target would be useful for hfsfuse which bundles utf8proc as a subtree and offers the ability to uninstall bundled libraries in its own Makefile.

`make uninstall` can be used to uninstall the libraries, utf8proc
header, and pkg-config file.
@stevengj
Copy link
Member

stevengj commented Jan 26, 2026

Thanks! It would be good to add a test for this — i.e. do a make install into a temporary directory, then make uninstall, and check that it is empty (except for empty subdirs like lib and include).

@0x09
Copy link
Author

0x09 commented Jan 26, 2026

Sure, let me know if something like this is what you had in mind, if so I'll add it to the tests dir and to make check.

#!/usr/bin/env bash

install_dir=$(mktemp -d)

if ! make install DESTDIR="$install_dir" ||
   [ ! -f "$install_dir/usr/local/include/utf8proc.h" ] ||
   [ ! -f "$install_dir/usr/local/lib/libutf8proc.a" ] ||
   [ ! -f "$install_dir/usr/local/lib/pkgconfig/libutf8proc.pc" ]; then
	echo "FAILED: make install" >&2
	rm -rf "$install_dir"
	exit 1
fi

if ! make uninstall DESTDIR="$install_dir" ||
   [ -n "$(find "$install_dir/usr/local/include" -type f )" ]; then
	echo "FAILED: make uninstall" >&2
	rm -rf "$install_dir"
	exit 1
fi

@stevengj
Copy link
Member

Overall that looks good. For the make install test it would be good to check against the MANIFEST file.

I would also probably just rely on #!/usr/bin/sh rather than bash.

@0x09
Copy link
Author

0x09 commented Jan 27, 2026

A minor issue with using the manifest as-is is that it doesn't take into account platform differences in the shared library name, so make check fails on macOS.
What we could do is have the test/install_uninstall target depend on the manifest target, and then reference Manifest.new in the test. This requires FIND be set to gfind on macOS due to the find syntax used when building the new manifest.
Since the Makefile already does platform detection for the shared library name and requires GNU find, it seems reasonable to just set FIND to this conditionally in the Makefile as well.

This seems like a reasonable solution to me so I'll push a new commit with these changes, but let me know if you know of a better way to do this.

Adds a script which validates the install and ensures that uninstall
removes all installed files based on the generated manifest file. This
is called as part of `make check`.

On macOS, generating the manifest requires use of GNU find, so this is
now set conditionally in the Makefile.
@0x09
Copy link
Author

0x09 commented Jan 27, 2026

The branch has been updated with the above changes.


OS := $(shell uname)
ifeq ($(OS),Darwin) # MacOS X
FIND=gfind
Copy link
Member

Choose a reason for hiding this comment

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

gfind is not installed by default on MacOS X. It would be better for us to use the POSIX find syntax.

Copy link
Author

Choose a reason for hiding this comment

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

This comes from the MANIFEST.new make target. It looks like there's no direct equivalent with BSD find, but the same manifest format can be generated portably using (somewhat lengthy) shell scripting:

MANIFEST.new:
	rm -rf tmp
	$(MAKE) install prefix=/usr DESTDIR=$(PWD)/tmp
	find tmp/usr -mindepth 1 | cut -d / -f 3- | while read -r file; do
	  if [ -L "$file" ]; then
	    printf "%s -> %s\n" "$file" "`readlink "$file"`";
	  elif [ -d "$file" ]; then
	    printf "%s/\n" "$file";
	  else
	    printf "%s\n" "$file"
	  fi
	done | LC_ALL=C sort
	rm -rf tmp

Let me know if you'd like me to go ahead and make that change.

However reading this made me realize that the install test added in this PR is circuitous since the manifest is generated by running make install in the first place, so I'll just remove that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's fine to remove the install test and leave MANIFEST.new as-is. It's only used for CI check on Ubuntu, currently.

Copy link
Author

Choose a reason for hiding this comment

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

So just to be clear, the uninstall test will not support running on macOS either in that case. If that's fine then the PR has been updated to remove the install test and the only other thing to do would be to remove this FIND assignment since it's unneeded.

0x09 added 2 commits January 28, 2026 17:17
This was based on the contents of the generated MANIFEST, but this
is generated by running `make install` itself, so this test was
circuitous.
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.

2 participants