On 2015-12-16 15:21:46, John Johansen wrote: > On 12/16/2015 03:17 PM, Tyler Hicks wrote: > > On 2015-12-16 23:46:23, Christian Boltz wrote: > >> Hello, > >> > >> Am Mittwoch, 16. Dezember 2015 schrieb Tyler Hicks: > >>> diff --git a/README b/README > >>> index 4ebd25d..4797b78 100644 > >>> --- a/README > >>> +++ b/README > >> > >>> @@ -104,7 +112,7 @@ $ make check # depends on the parser having been > >>> built first $ make install > >>> > >>> > >>> -[Note that for the parser and the utils, if you only with to > >>> build/use > >>> +[Note that for the parser, binutils, and utils, if you > >>> only with to > >> > >> unrelated, but please s/with/wish/ > > > > I'll make the change in my local copy of this commit. > > > >> > >>> build/use some of the locale languages, you can override > >>> the default by passing the LANGS arguments to make; e.g. make all > >>> install "LANGS=en_US fr".] > >>> > >>> diff --git a/binutils/Makefile b/binutils/Makefile > >>> new file mode 100644 > >>> index 0000000..c4c4c2e > >>> --- /dev/null > >>> +++ b/binutils/Makefile > >> > >> My general note that this looks like a copy of parser/Makefile and > >> splitting out common parts to something like common/Make-c.rules would > >> make sense still stands - but this can be done as a separate patch. > > > > I agree that cleaning up the common Makefile snippets is a good idea. > > However, it is not work that I signed up for when offering to take over > > this patch and it isn't something that I'll be able to get to any time > > soon. > > > >> Also, please make sure that the (mostly acked, but not yet commited) > >> changes for parser/Makefile also get applied to binutils/Makefile to keep > >> the difference small. > >> > >>> +.PHONY: install-rhel4 > >>> +install-rhel4: install-redhat > >>> + > >>> +.PHONY: install-redhat > >>> +install-redhat: > >>> + > >>> +.PHONY: install-suse > >>> +install-suse: > >>> + > >>> +.PHONY: install-slackware > >>> +install-slackware: > >>> + > >>> +.PHONY: install-debian > >>> +install-debian: > >>> + > >>> +.PHONY: install-unknown > >>> +install-unknown: > >>> + > >>> +INSTALLDEPS=arch > >>> + > >>> +ifndef DISTRO > >>> +DISTRO=$(shell if [ -f /etc/slackware-version ] ; then \ > >>> + echo slackware ; \ > >>> + elif [ -f /etc/debian_version ] ; then \ > >>> + echo debian ;\ > >>> + elif which rpm > /dev/null ; then \ > >>> + if [ "$(rpm --eval '0%{?suse_version}')" != "0" ] ; then \ > >>> + echo suse ;\ > >>> + elif [ "$(rpm --eval '%{_host_vendor}')" = redhat ] ; then > >>> \ + echo rhel4 ;\ > >>> + elif [ "$(rpm --eval '0%{?fedora}')" != "0" ] ; then \ > >>> + echo rhel4 ;\ > >>> + else \ > >>> + echo unknown ;\ > >>> + fi ;\ > >>> + else \ > >>> + echo unknown ;\ > >>> + fi) > >>> +endif > >>> + > >>> +ifdef DISTRO > >>> +INSTALLDEPS+=install-$(DISTRO) > >>> +endif > >> > >> All the install-$distribution (and also the distro detection) seems to > >> be unused. Do we really need it? > > > > Probably not. Maybe John can shed some light on why this was included? > > cargo culting, I started with the parser Makefile and didn't spend the > time to look into this any more
Ok, instead of revising this patch once more, I'm going to commit it as-is (with all the other changes cboltz pointed out) and then submit a follow up patch to remove the Makefile cruft. > > > >> Note that we should probably keep "INSTALLDEPS=arch". > >> (Again, this can be done in a follow-up patch.) > >> > >>> +clean: pod_clean > >>> + rm -f core core.* *.o *.s *.a *~ *.gcda *.gcno > >>> + rm -f gmon.out > >>> + rm -f $(TOOLS) $(TESTS) > >>> + rm -f $(NAME)*.tar.gz $(NAME)*.tgz > >> > >> Show me where this tarball gets built, and I'll accept that we need to > >> delete it ;-) (I'd guess this sniplet is from the good old days when > >> you [1] still had separate tarballs for profiles, utils, parser etc.) > > > > I will remove that line in my local copy of this commit. > > > yeah vestiges of separate tar balls > > >>> diff --git a/binutils/aa-enabled.pod b/binutils/aa-enabled.pod > >>> new file mode 100644 > >>> index 0000000..217bc65 > >>> --- /dev/null > >>> +++ b/binutils/aa-enabled.pod > >> > >>> +=head1 DESCRIPTION > >>> + > >>> +B<aa-enabled> is used to determine if AppArmor is enabled and > >>> enforcing +policy. > >> > >> With the (in comparison to aa-status --enabled) removal of the check if > >> at least one profile is loaded, the "and enforcing policy" part looks a > >> bit confusing to me. > > > > Agreed. I'll remove the "and enforcing policy" portion of that line. > > > >> > >> Removing that check is ok, I just want to be sure nobody misunderstands > >> what aa-enabled does. The release notes should also make that clear, and > >> explain that aa-enabled mostly replaces aa-status --enabled, but will > >> return 0 even if no profiles are loaded (which is probably an improvement > >> when used in package scripts). > >> > >>> +=head1 EXIT STATUS > >> ... > >>> +=item 2: > >>> + > >>> +intentionally not used as an B<aa-enabled> exit status. > >> > >> I wonder if we should explain the reason for this (see above). > > > > Meh. A user isn't going to care outside of knowing that it isn't a > > possible exit status. > > > >> > >>> diff --git a/binutils/aa_enabled.c b/binutils/aa_enabled.c > >>> new file mode 100644 > >>> index 0000000..3ca84b0 > >>> --- /dev/null > >>> +++ b/binutils/aa_enabled.c > >> > >> Looks good - but I'll let acking to someone who understands C better ;-) > > > > Thanks for the review! > > > > John wrote the patch. I've looked closely over all of the patch and you > > have too. I think we have enough review to commit the patch once we hear > > back from John regarding the install targets in the Makefile. > > > sure, you can have my Acked-by for your bits :) Thanks! Tyler
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor