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

Build phase 6: %install cleanup #233

Closed
wants to merge 4 commits into from
Closed

Conversation

pspacek
Copy link
Contributor

@pspacek pspacek commented Nov 11, 2016

This patch set is based on phase 5 - PR #226.

Now all the installation steps (except Python2/3) are handled by make install.

Python 2/3 support will deserve separate PR.

@pspacek
Copy link
Contributor Author

pspacek commented Nov 11, 2016

Rebased on top of current master.

@MartinBasti
Copy link
Contributor

Build failed

Failed to open: 'freeipa.spec.in', not a valid spec file.

@@ -19,6 +19,10 @@ SUBDIRS = \
$(NULL)

install-exec-local:
mkdir -p $(DESTDIR)$(IPA_SYSCONF_DIR)/custodia
chmod 700 $(DESTDIR)$(IPA_SYSCONF_DIR)/custodia
Copy link
Member

Choose a reason for hiding this comment

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

Please use $(INSTALL) -d -m 700 -o root -g root here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to be consistent but good point, I will change it to $(INSTALL). -o root cannot be used as explained below. Changing file ownership is job of package manager.

htmldatadir = $(datarootdir)/ipa/html
install-data-hook:
$(INSTALL) -d -D -m 755 $(DESTDIR)$(htmldatadir)
RELATIVE_PATH=$$(python -c "import os.path; print(os.path.relpath('$(appdir)', '$(htmldatadir)'))"); \
Copy link
Member

Choose a reason for hiding this comment

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

You don't need Python here. ln has -r for create symbolic links relative to link location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I will use that.

@@ -34,3 +34,6 @@ MAINTAINERCLEANFILES = \
*~ \
Makefile.in
$(NULL)

install-data-hook:
$(INSTALL) -d -D -m 755 $(DESTDIR)$(appdir)/js/plugins
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure you install files and directories as root:root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make install cannot install files as root (or any other user) because make intall is typically called as unprivileged user by the rpmbuild.

@pspacek
Copy link
Contributor Author

pspacek commented Nov 11, 2016

Failed to open: 'freeipa.spec.in', not a valid spec file.

Damn it! I added the last commit with comment in SPEC file and did not run tests on that. Surprise surprise, RPM is PARSIN TEXT INSIDE COMMENTS and if you put %install into comment, guess what: It will blow up! Grrrr. (Another joke is that DNF builddep is throwing away error descrption from RPM SPEC file parser.)

@pspacek
Copy link
Contributor Author

pspacek commented Nov 11, 2016

This version fixes the fixable issues, i.e. everything mentioned above except changing file ownership.

@pspacek
Copy link
Contributor Author

pspacek commented Nov 11, 2016

@tiran Please re-review and set review status accordingly. Thanks!

@pspacek
Copy link
Contributor Author

pspacek commented Nov 11, 2016

I've fixed incorrect use of -D in install calls above.

@MartinBasti
Copy link
Contributor

@tiran do you agree with changes?

@tiran tiran added the ack Pull Request approved, can be merged label Nov 15, 2016
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants