[merged] Add Makefile #164
Conversation
|
LGTM but you might want to remove the |
|
Removed Source* |
|
This should install binaries in /usr/local/ instead of /usr ? Who defines DESTDIR |
|
It is defined in the RPM. Or anyone who installs it. Whether it installs to /usr/local versus /usr is debatable. Especially where the /etc files are shred. |
| install -D -m 644 docker-storage-setup.conf ${DSSLIBDIR}/docker-storage-setup | ||
| install -D -m 644 docker-storage-setup-override.conf ${SYSCONFDIR}/docker-storage-setup | ||
| install -D -m 755 libdss.sh ${DSSLIBDIR}/libdss.sh | ||
| install -D -m 755 dss-child-read-write.sh ${DSSLIBDIR}/dss-child-read-write |
There was a problem hiding this comment.
Why not preserce timestamps using -p option
There was a problem hiding this comment.
Most likely not going to matter, since this is more about packaging in an RPM which is not going to preserve timestamps. I don't think these are usually preserved in a Makefile.
|
Looks like shishir forgot to add |
| install -D -m 755 docker-storage-setup.sh ${BINDIR}/docker-storage-setup | ||
| install -D -m 644 docker-storage-setup.service ${UNITDIR}/docker-storage-setup.service | ||
| install -D -m 644 docker-storage-setup.conf ${DSSLIBDIR}/docker-storage-setup | ||
| install -D -m 644 docker-storage-setup-override.conf ${SYSCONFDIR}/docker-storage-setup |
There was a problem hiding this comment.
Don't like the idea of overwriting user's config file /etc/sysconfig/docker-storage-setup. Can't we do this only if /etc/sysconfig/docker-storage-setup is not present.
|
That can be done in the spec file Not usually done in the Makefile. |
|
spec file already has it.
I think it would be a real surprise to overwrite user's config file. I don't expect |
|
Do you have any examples of packages that don't? |
|
We can add a -b to the install line which will backup the older version. |
|
I think something like this will make sense. Overwrite /etc/sysconfig/docker-storage-setup only if it does not exist. https://github.com/openssh/openssh-portable/blob/master/Makefile.in#L346 |
|
@rhvgoyal PTAL |
| @@ -54,9 +38,12 @@ install -p -m 755 %{SOURCE5} %{buildroot}/%{dsslibdir}/dss-child-read-write | |||
| %files | |||
| %{_unitdir}/docker-storage-setup.service | |||
| %{_bindir}/docker-storage-setup | |||
| %{dsslibdir}/docker-storage-setup | |||
| %config(noreplace) %{_sysconfdir}/sysconfig/docker-storage-setup-override | |||
There was a problem hiding this comment.
I thought %files section is supposed to keep a list of files which are installed by package on system. And docker-storage-setup-override is not installed by package on the host.
| %config(noreplace) %{_sysconfdir}/sysconfig/docker-storage-setup | ||
| %dir %{dsslibdir} |
There was a problem hiding this comment.
Why do we need %dir. Following webpage seems to say,
if a directory is specified in the %files list, the contents of that directory, and the contents of every directory under it, will automatically be included in the package
http://www.rpm.org/max-rpm-snapshot/s1-rpm-inside-files-list-directives.html
And we don't want that. We are mentioning each file individually.
| @@ -54,9 +38,12 @@ install -p -m 755 %{SOURCE5} %{buildroot}/%{dsslibdir}/dss-child-read-write | |||
| %files | |||
| %{_unitdir}/docker-storage-setup.service | |||
| %{_bindir}/docker-storage-setup | |||
| %{dsslibdir}/docker-storage-setup | |||
There was a problem hiding this comment.
Why are we removing above. We put default config options in /usr/lib/docker-storage-setup/ and want to overwrite it on all package upgrades.
| @@ -30,17 +24,7 @@ as the root logical volume and partition table. | |||
| %build | |||
|
|
|||
| %install | |||
| install -d %{buildroot}%{_bindir} | |||
There was a problem hiding this comment.
So previously, buildroot will define the root. Now we seem to be using DESTDIR as replacement for that. Who sets DESTDIR?
There was a problem hiding this comment.
There is no equivalent of _bindir though as we are hardcoding it to bin
| install -d %{buildroot}%{_bindir} | ||
| install -p -m 755 %{SOURCE0} %{buildroot}%{_bindir}/docker-storage-setup | ||
| install -d %{buildroot}%{_unitdir} | ||
| install -p -m 644 %{SOURCE1} %{buildroot}%{_unitdir} |
There was a problem hiding this comment.
If we are getting rid of macros like _unitdir, are we not going to be broken if tomorrow systemd changes location of unit files and this macro starts expanding to a different value.
|
LGTM |
|
📌 Commit 8fd6747 has been approved by |
|
☀️ Test successful - status-atomicjenkins |
No description provided.