PR#22: Remove %*name and %installbase macros - rpms/ansible-collection-microsoft-sql

rpms / ansible-collection-microsoft-sql

#22 Remove %*name and %installbase macros

Merged 2 years ago by spetros. Opened 2 years ago by gotmax23.

rpms/ gotmax23/ansible-collection-microsoft-sql remove_name_macros into rawhide

Use %ansible_collection_filelist in files

Maxwell G • 2 years ago

b80b156

Replace %{installbase} with %{ansible_roles_dir}

Maxwell G • 2 years ago

07382af

ansible-collection-microsoft-sql.spec

file modified

+14 -16

		`@@ -14,7 +14,6 @@`
		`%global collection_rolename server`
		`%global legacy_rolename %{collection_namespace}.sql-server`

		`- %global installbase %{_datadir}/ansible/roles`
		`%global _pkglicensedir %{_licensedir}/%{name}`

		`Requires: linux-system-roles`
		`@@ -108,35 +107,35 @@`
		`popd`

		`%install`
		`- mkdir -p %{buildroot}%{installbase}`
		`+ mkdir -p %{buildroot}%{ansible_roles_dir}`

		`# Copy role in legacy format and rename rolename in tests`
		`- cp -pR %{rolename} "%{buildroot}%{installbase}/%{legacy_rolename}"`
		`+ cp -pR %{rolename} "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}"`
		`sed -i "s/linux-system-roles\.%{rolename}/%{legacy_rolename}/g" \`
		`- %{buildroot}%{installbase}/%{legacy_rolename}/tests/*.yml`
		`+ %{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/tests/*.yml`

		`# Copy README, COPYING, and LICENSE files to the corresponding directories`
		`mkdir -p %{buildroot}%{_pkglicensedir}`
		`mkdir -p "%{buildroot}%{_pkgdocdir}/%{legacy_rolename}"`
		`- cp -p "%{buildroot}%{installbase}/%{legacy_rolename}/README.md" \`
		`+ cp -p "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/README.md" \`
		`"%{buildroot}%{_pkgdocdir}/%{legacy_rolename}"`
		`- cp -p "%{buildroot}%{installbase}/%{legacy_rolename}/README.html" \`
		`+ cp -p "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/README.html" \`
		`"%{buildroot}%{_pkgdocdir}/%{legacy_rolename}"`
		`- if [ -f "%{buildroot}%{installbase}/%{legacy_rolename}/COPYING" ]; then`
		`- cp -p "%{buildroot}%{installbase}/%{legacy_rolename}/COPYING" \`
		`+ if [ -f "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/COPYING" ]; then`
		`+ cp -p "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/COPYING" \`
		`"%{buildroot}%{_pkglicensedir}/%{legacy_rolename}.COPYING"`
		`fi`
		`- if [ -f "%{buildroot}%{installbase}/%{legacy_rolename}/LICENSE" ]; then`
		`- cp -p "%{buildroot}%{installbase}/%{legacy_rolename}/LICENSE" \`
		`+ if [ -f "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/LICENSE" ]; then`
		`+ cp -p "%{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/LICENSE" \`
		`"%{buildroot}%{_pkglicensedir}/%{legacy_rolename}.LICENSE"`
		`fi`

		`# Remove dot files`
		`- rm -r %{buildroot}%{installbase}//.[A-Za-z]`
		`- rm -r %{buildroot}%{installbase}/%{legacy_rolename}/tests/.[A-Za-z]*`
		`+ rm -r %{buildroot}%{ansible_roles_dir}//.[A-Za-z]`
		`+ rm -r %{buildroot}%{ansible_roles_dir}/%{legacy_rolename}/tests/.[A-Za-z]*`

		`# Remove the molecule directory`
		`- rm -r %{buildroot}%{installbase}/*/molecule`
		`+ rm -r %{buildroot}%{ansible_roles_dir}/*/molecule`

		`# Install collection`
		`pushd .collections/ansible_collections/%{collection_namespace}/%{collection_name}/`
		`@@ -169,11 +168,10 @@`
		`popd`
		`%endif`

		`- %files`
		`+ %files -f %{ansible_collection_filelist}`
		`%{_pkgdocdir}`
		`%license %{_pkglicensedir}`
		`- %{ansible_collection_files}`
		`- %{installbase}/%{legacy_rolename}`
		`+ %{ansible_roles_dir}/%{legacy_rolename}`

		`%if %{with collection_artifact}`
		`%files collection-artifact`

gotmax23 commented 2 years ago

%installbase is no longer needed. ansible-packaging now provides an
%ansible_roles_dir that should be used instead.

%collection_namespace and %collection_name used to be needed by
%ansible_collecion_install and %ansible_collection_files, but I have
removed this requirement in the latest version of ansible-packaging.
Therefore, all instances of %collection_namespace and %collection_name
were replaced with their literal values.

As part of this change, I've added a new %ansible_collection_filelist
macro which should be used instead of %ansible_collection_files.

All instances of %legacy_rolename, %rolename, and %collection_rolename
were also replaced with their literal values.

At least in my opinion, removing the %*name macros makes the specfile
much more readable. It is also more concise; there is no need to define
all of the macros, and the literal values are shorter than the macro
names.

spetros commented 2 years ago

Hi @gotmax23 ,
thank you for your contribution. Replacing %installbase with %ansible_roles_dir and %ansible_collection_files with %ansible_collection_filelist.
I would like to keep %legacy_rolename, %rolename, %collection_namespace, and %collection_rolename, removing them would make it impossible to re-use this spec file for a future RPMs with similar Ansible content. It also reduces the probability of typos in those strings. Our team mainly places roles into the linux-system-roles RPM, which is built specifically to simplify the process of adding new roles. However, some roles that do not belong under linux-system-roles might need its own RPMs. Those roles would be able to re-use this spec file, replace a couple of strings there, and it would work, but if we remove the macros, re-using the spec file would be a much more manual process.
What do you think? Would you be able to return %*name macros?
Thank you

gotmax23 commented 2 years ago

Hi @gotmax23 ,
thank you for your contribution.

Sure!

I would like to keep %legacy_rolename, %rolename, %collection_namespace, and %collection_rolename, removing them would make it impossible to re-use this spec file for a future RPMs with similar Ansible content.

You could always use find and replace, so reuse wouldn't be impossible, but yes, it would be a little less straightforward. Personally, I find all of those name variables quite confusing. I would say the trade off is worth it, but this kind of thing is up to the maintainers; there's no formal guideline one way or another.

What do you think? Would you be able to return %*name macros?

Yes, I could do that.

spetros commented 2 years ago

@gotmax23 please return %*name macros, only keep replacing %installbase with %ansible_roles_dir and %ansible_collection_files with %ansible_collection_filelist and I will merge

rebased onto 07382af

2 years ago

gotmax23 commented 2 years ago

Thanks for the reminder. Please see my latest changes.

spetros commented 2 years ago

I am trying to test it locally but fedpkg --release f36 mockbuild fails for me with error: Could not open %files file /builddir/build/BUILD/auto-maintenance-cdc706f14614ef5e80bbce8db10beb369e889df9/%{ansible_collection_filelist}: No such file or directory.
The ansible_collection_filelist macro does not expand for me, am I missing something?

gotmax23 commented 2 years ago

Try fedpkg --release f36 mockbuild --enablerepo=local. The ansible-packaging update which adds this macro is still in testing, but it's in the buildroot and should be pushed to the mirrors tomorrow.

spetros commented 2 years ago

All works, rpmdiff -iT between previous and updated specs returns zero changes in the RPMs built, merging.