tools/pygrub: Drop compatibility symlink

[PATCH] tools/pygrub: Drop compatibility symlink

Posted by Andrew Cooper 5 months, 1 week ago

This was declared deprecated in commit 10c88f1c18b7 ("tools: Install pv
bootloaders in libexec rather than /usr/bin") in 2012

Take it out fully now, 11 years later.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
---
 CHANGELOG.md          | 3 +++
 tools/pygrub/Makefile | 6 ------
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ecebb9f686a..36a8ef89d8e4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### Removed
 - caml-stubdom.  It hasn't built since 2014, was pinned to Ocaml 4.02, and has
   been superseded by the MirageOS/SOLO5 projects.
+- /usr/bin/pygrub symlink.  This was deprecated in Xen 4.2 (2012) but left for
+  compatibility reasons.  VMs configured with bootloader="/usr/bin/pygrub"
+  should be updated to just bootloader="pygrub".
 
 ## [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0) - 2023-11-16
 
diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
index 4963bc89c6ed..d5e291ea0619 100644
--- a/tools/pygrub/Makefile
+++ b/tools/pygrub/Makefile
@@ -22,15 +22,9 @@ install: all
 	$(setup.py) install --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \
 		--root="$(DESTDIR)" --force
 	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(LIBEXEC_BIN)/pygrub
-	set -e; if [ $(bindir) != $(LIBEXEC_BIN) -a \
-	             "`readlink -f $(DESTDIR)/$(bindir)`" != \
-	             "`readlink -f $(LIBEXEC_BIN)`" ]; then \
-	    ln -sf $(LIBEXEC_BIN)/pygrub $(DESTDIR)/$(bindir); \
-	fi
 
 .PHONY: uninstall
 uninstall:
-	rm -f $(DESTDIR)/$(bindir)/pygrub
 	if [ -e $(INSTALL_LOG) ]; then \
 		cat $(INSTALL_LOG) | xargs -i rm -f $(DESTDIR)/{}; \
 	fi

base-commit: f96e2f64576cdbb147391c7cb399d393385719a9
-- 
2.30.2

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by Anthony PERARD 4 months, 3 weeks ago

On Thu, Nov 23, 2023 at 04:30:23PM +0000, Andrew Cooper wrote:
> This was declared deprecated in commit 10c88f1c18b7 ("tools: Install pv
> bootloaders in libexec rather than /usr/bin") in 2012

This commit only speak about wanting to deprecate the full path to
`pygrub`, and calling something deprecated in a commit message alone
isn't very friendly. But there's a better commit calling for the
deprecation:
c31d6a7ee2ea ("libxl: Warn that /usr/bin/pygrub is deprecated")
even if it's only in a libxl log message at run time.

I hope we have a better schema to deprecate things.

> Take it out fully now, 11 years later.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Beside the commit message that could call for a better commit, patch
looks good:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by Andrew Cooper 4 months, 3 weeks ago

On 11/12/2023 5:07 pm, Anthony PERARD wrote:
> On Thu, Nov 23, 2023 at 04:30:23PM +0000, Andrew Cooper wrote:
>> This was declared deprecated in commit 10c88f1c18b7 ("tools: Install pv
>> bootloaders in libexec rather than /usr/bin") in 2012
> This commit only speak about wanting to deprecate the full path to
> `pygrub`, and calling something deprecated in a commit message alone
> isn't very friendly. But there's a better commit calling for the
> deprecation:
> c31d6a7ee2ea ("libxl: Warn that /usr/bin/pygrub is deprecated")
> even if it's only in a libxl log message at run time.
>
> I hope we have a better schema to deprecate things.

Oh, I'd not even spotted that one.

This patch was actually triggered by newer versions of RPM objecting to
absolute symlinks.

>
>> Take it out fully now, 11 years later.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Beside the commit message that could call for a better commit, patch
> looks good:
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks.  I'll adjust the commit message.

~Andrew

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by Andrew Cooper 4 months, 3 weeks ago

Ping.

On 23/11/2023 4:30 pm, Andrew Cooper wrote:
> This was declared deprecated in commit 10c88f1c18b7 ("tools: Install pv
> bootloaders in libexec rather than /usr/bin") in 2012
>
> Take it out fully now, 11 years later.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> ---
>  CHANGELOG.md          | 3 +++
>  tools/pygrub/Makefile | 6 ------
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index 4ecebb9f686a..36a8ef89d8e4 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>  ### Removed
>  - caml-stubdom.  It hasn't built since 2014, was pinned to Ocaml 4.02, and has
>    been superseded by the MirageOS/SOLO5 projects.
> +- /usr/bin/pygrub symlink.  This was deprecated in Xen 4.2 (2012) but left for
> +  compatibility reasons.  VMs configured with bootloader="/usr/bin/pygrub"
> +  should be updated to just bootloader="pygrub".
>  
>  ## [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0) - 2023-11-16
>  
> diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
> index 4963bc89c6ed..d5e291ea0619 100644
> --- a/tools/pygrub/Makefile
> +++ b/tools/pygrub/Makefile
> @@ -22,15 +22,9 @@ install: all
>  	$(setup.py) install --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \
>  		--root="$(DESTDIR)" --force
>  	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(LIBEXEC_BIN)/pygrub
> -	set -e; if [ $(bindir) != $(LIBEXEC_BIN) -a \
> -	             "`readlink -f $(DESTDIR)/$(bindir)`" != \
> -	             "`readlink -f $(LIBEXEC_BIN)`" ]; then \
> -	    ln -sf $(LIBEXEC_BIN)/pygrub $(DESTDIR)/$(bindir); \
> -	fi
>  
>  .PHONY: uninstall
>  uninstall:
> -	rm -f $(DESTDIR)/$(bindir)/pygrub
>  	if [ -e $(INSTALL_LOG) ]; then \
>  		cat $(INSTALL_LOG) | xargs -i rm -f $(DESTDIR)/{}; \
>  	fi
>
> base-commit: f96e2f64576cdbb147391c7cb399d393385719a9

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by Julien Grall 4 months, 3 weeks ago

Hi,

On 08/12/2023 15:13, Andrew Cooper wrote:
> Ping.

I noticed Anthony is not CCed (scripts/get_maintainer.pl reports him and 
Wei). The same for CHANGELOG. This should have been Henry (and soon 
Oleksii).

Is this intended?

> 
> On 23/11/2023 4:30 pm, Andrew Cooper wrote:
>> This was declared deprecated in commit 10c88f1c18b7 ("tools: Install pv
>> bootloaders in libexec rather than /usr/bin") in 2012
>>
>> Take it out fully now, 11 years later.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> ---
>>   CHANGELOG.md          | 3 +++
>>   tools/pygrub/Makefile | 6 ------

See above, I think this code is under the remit of Anthony. You haven't 
CCed him so no surprise no-one answered.

It is unclear why "THE REST" was CCed. I called 
scripts/get_maintainers.pl on the patch and only Henry/Anthony/Wei
was listed.

I have CCed Anthony. I can review give a try to review it if you don't 
get any answer from Anthony by mid-next week.

>>   2 files changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>> index 4ecebb9f686a..36a8ef89d8e4 100644
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
>>   ### Removed
>>   - caml-stubdom.  It hasn't built since 2014, was pinned to Ocaml 4.02, and has
>>     been superseded by the MirageOS/SOLO5 projects.
>> +- /usr/bin/pygrub symlink.  This was deprecated in Xen 4.2 (2012) but left for
>> +  compatibility reasons.  VMs configured with bootloader="/usr/bin/pygrub"
>> +  should be updated to just bootloader="pygrub".
>>   
>>   ## [4.18.0](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=shortlog;h=RELEASE-4.18.0) - 2023-11-16
>>   
>> diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile
>> index 4963bc89c6ed..d5e291ea0619 100644
>> --- a/tools/pygrub/Makefile
>> +++ b/tools/pygrub/Makefile
>> @@ -22,15 +22,9 @@ install: all
>>   	$(setup.py) install --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \
>>   		--root="$(DESTDIR)" --force
>>   	$(INSTALL_PYTHON_PROG) src/pygrub $(DESTDIR)/$(LIBEXEC_BIN)/pygrub
>> -	set -e; if [ $(bindir) != $(LIBEXEC_BIN) -a \
>> -	             "`readlink -f $(DESTDIR)/$(bindir)`" != \
>> -	             "`readlink -f $(LIBEXEC_BIN)`" ]; then \
>> -	    ln -sf $(LIBEXEC_BIN)/pygrub $(DESTDIR)/$(bindir); \
>> -	fi
>>   
>>   .PHONY: uninstall
>>   uninstall:
>> -	rm -f $(DESTDIR)/$(bindir)/pygrub
>>   	if [ -e $(INSTALL_LOG) ]; then \
>>   		cat $(INSTALL_LOG) | xargs -i rm -f $(DESTDIR)/{}; \
>>   	fi
>>
>> base-commit: f96e2f64576cdbb147391c7cb399d393385719a9
> 

Cheers,

-- 
Julien Grall

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by George Dunlap 4 months, 3 weeks ago

On Fri, Dec 8, 2023 at 3:36 PM Julien Grall <julien@xen.org> wrote:

> See above, I think this code is under the remit of Anthony. You haven't
> CCed him so no surprise no-one answered.

Additionally, my old citrix address *was* cc'd, but I only have the
'ping', not the original email in my work mailbox.

I'm tempted to suggest adding a hack to libxl, so that /usr/bin/pygrub
automatically falls back to the libexec directory if /usr/bin/pygrub
doesn't exist.  (I'd be happy to code something up if we decided to go
that route.). OTOH, I can see the argument that 11 years and the
CHANGELOG entry is enough.

 -George

Re: [PATCH] tools/pygrub: Drop compatibility symlink

Posted by Anthony PERARD 4 months, 3 weeks ago

On Fri, Dec 08, 2023 at 03:43:16PM +0000, George Dunlap wrote:
> On Fri, Dec 8, 2023 at 3:36 PM Julien Grall <julien@xen.org> wrote:
> 
> > See above, I think this code is under the remit of Anthony. You haven't
> > CCed him so no surprise no-one answered.
> 
> Additionally, my old citrix address *was* cc'd, but I only have the
> 'ping', not the original email in my work mailbox.
> 
> I'm tempted to suggest adding a hack to libxl, so that /usr/bin/pygrub
> automatically falls back to the libexec directory if /usr/bin/pygrub
> doesn't exist.  (I'd be happy to code something up if we decided to go
> that route.). OTOH, I can see the argument that 11 years and the
> CHANGELOG entry is enough.

Well, at least you have printed a warning log message to all of those
whom still use the full old path to pygrub, with
    c31d6a7ee2ea ("libxl: Warn that /usr/bin/pygrub is deprecated")
So, I don't think more than that is needed. At least, now they would
also get an error saying /usr/bin/pygrub wasn't found, I think.

Cheers,

-- 
Anthony PERARD