#3100 PR `open`: OUTPUT=PXE: Support for downloading kernel and initrd by http¶

Labels: enhancement

robertdahlem opened issue at 2023-12-05 15:40:¶

Pull Request Details:¶

Type: Enhancement
Impact: Low
How was this pull request tested?
By using it in production.
Description of the changes in this pull request:

This patch adds support for downloading kernel and initrd by http (instead of tftp) when using PXE. It was motivated by long download times for 700 MB initrds over tftp. Tftp uses UDP and doesn't have a send window like TCP, meaning TCP can send packets up to the size of the send window without having received acknowledges for previous packets und thus is much better at saturating the wire compared to UDP, where each packet has to be acknowledged before the next packet can be sent.

Obviously you need a http server for this and mkrescue must be able to write there. Details of the configuration are in default.conf. The main changes are:

PXE_TFTP_URL: deprecated, use PXE_TFTP_UPLOAD_URL
PXE_HTTP_URL: deprecated, use PXE_HTTP_DOWNLOAD_URL
PXE_TFTP_UPLOAD_URL: where the TFTP files shall be put
PXE_HTTP_UPLOAD_URL: where the HTTP files shall be put
PXE_HTTP_DOWNLOAD_URL: where the HTTP files shall be downloaded from

A requirement is switching from pxelinux.0 to lpxelinux.0. It is a drop in replacement for pxelinux.0 which supports http downloads and should be included in any recent pxelinux package. Recent meaning "after 2019". :-)

jsmeix commented at 2023-12-06 13:58:¶

@robertdahlem
thank you for your contribution to improve ReaR!

I am not a PXE user but I will have a look
at your pull request tomorrow (as time permits).

pcahyna commented at 2023-12-06 14:05:¶

@robertdahlem thank you for you contribution! Unfortunately I am not a PXE user so I can not test it not have much specific comments.
From looking at the code, I see

It's compatible, just replace pxeconfig.0 in your TFTP directory or set bootfile-name to lpxeconfig.0 in your DHCP server.

but in your description, you write:

A requirement is switching from pxelinux.0 to lpxelinux.0.

Is the difference intentional?

Also, why the PXE_SCHEME option name? To me it does not look descriptive. For me, "scheme" is the http part of the URL.

Regarding the unfinished RPM builds in CI, don't worry, that's some general Fedora COPR issue.
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/RANEB44PFVY3C2LCUAT2LSTQ3QXB7Y5C/#RANEB44PFVY3C2LCUAT2LSTQ3QXB7Y5C

robertdahlem commented at 2023-12-06 14:28:¶

@pcahyna

From looking at the code, I see

It's compatible, just replace pxeconfig.0 in your TFTP directory or set bootfile-name to lpxeconfig.0 in your DHCP server.

but in your description, you write:

A requirement is switching from pxelinux.0 to lpxelinux.0.

Is the difference intentional?

It don't see a difference. http downloads require lpxeconfig.0. You can switch to that in at least two ways:

Copy lpxeconfig.0 over pxeconfig.0 in /var/lib/tftpboot of the TFTP server. It is a compatible pxeconfig.0 replacement, just enhanced with http
Use the DHCP option "bootfile name (67)" to request lpxeconfig.0 instead of pxeconfig.0

The former is a bit more intrusive because it affects all PXE boots. Shouldn't be a problem because it is compatible (according to what the SYSLINUX/PXELINUX people say and what I tested).

The latter typically affects only single hosts and you need to replace this option for all affected hosts. Could be more work.

Also, why the PXE_SCHEME option name? To me it does not look descriptive. For me, "scheme" is the http part of the URL.

You are right, only "http://" is actually a scheme. It came to my mind because it supports a scheme change (from implicit tftp to explicit http). My heart is not attached to this specific name. It could very well be something like PXE_HTTP_PREFIX or anything else you propose.

pcahyna commented at 2023-12-06 14:44:¶

@pcahyna

From looking at the code, I see

It's compatible, just replace pxeconfig.0 in your TFTP directory or set bootfile-name to lpxeconfig.0 in your DHCP server.

but in your description, you write:

A requirement is switching from pxelinux.0 to lpxelinux.0.

Is the difference intentional?

It don't see a difference. http downloads require lpxeconfig.0. You can switch to that in at least two ways:
1. Copy lpxeconfig.0 over pxeconfig.0 in /var/lib/tftpboot of the TFTP server. It is a compatible pxeconfig.0 replacement, just enhanced with http

2. Use the DHCP option "bootfile name (67)" to request lpxeconfig.0 instead of pxeconfig.0

I see a difference between pxelinux.0 (PR description) and pxeconfig.0 (code comment).

robertdahlem commented at 2023-12-06 15:03:¶

@pcahyna

I see a difference between pxelinux.0 (PR description) and pxeconfig.0 (code comment).

Oops, sorry! Replace everything I ever said about (l)pxeconfig.0 with (l)pxelinux.0

I'll send an update.

jsmeix commented at 2023-12-07 12:35:¶

@robertdahlem
perhaps a dumb question (but as non PXE user
I know basically nothing how PXE works):

Could you explain how far
https://github.com/rear/rear/pull/2738
"Support for http sources when using PXE"
is related ro unrelated to this issue here?

robertdahlem commented at 2023-12-07 14:08:¶

@jsmeix

@robertdahlem perhaps a dumb question (but as non PXE user I know basically nothing how PXE works):
Could you explain how far #2738 "Support for http sources when using PXE" is related ro unrelated to this issue here?

Perhaps first some words how PXE works (basically, not all the gory details).

The system boots and when requested to, instead of reading what is on the disk it sends a DHCP request ("network boot" or "PXE boot"). It should get a DHCP reply with IP/netmask, default gateway, perhaps DNS and NTP servers.

For PXE to work DHCP also needs to send a "next server" (the TFTP server) and a "boot file". The boot file usually is pxelinux.0 from the SYSLINUX project. PXE downloads this file with TFTP and starts it. pxelinux.0 tries to download several things from the TFTP servers subdirectory pxelinux.cfg: a file with its UUID, a file with its MAC address, a file with its IP address (in hex) and so on. At least one of these should be a symbolic link to a configuration file which happens to be a GRUB menu configuration file. pxelinux.0 downloads this file and presents the user with the GRUB menu.

The user then requests something from the menu which leads to GRUB configuration items like kernel and initrd (with parameters). pxelinux.0 downloads both with TFTP from the "next server" and starts them. System running, job done. In the case of ReaR you get a system with pretty much the same as what is in an ISO: all the things you need to run "rear recover".

ReaR supports this by uploading a configuration and symbolic links to PXE_CONFIG_URL and kernel/initrd to PXE_TFTP_URL.

Unfortunately, nowadays initrd is typically a 700+ MB thing because it includes all available kernel modules to support recovery on different hardware. TFTP is very slow because it uses UDP and needs to acknowledge Every. Single. Packet. before downloading the next packet. I see initrd download times of 6 up to 8 minutes in a 10 Gbits/s environment (where you would expect a second or two).

HTTP is faster because it uses TCP and can send packages without waiting for acknowledgement of all previous packets.

So, if you prefix the kernel and initrd arguments with http://... you can download both a lot faster. pxelinux.0 does not understand http://.., thus the need to switch to lpxelinux.0. #2738 both and #3100 both support this.

Now for your question:

I have not been aware of #2738 because in my production environment I use what RHEL 8 delivers and that is ReaR 2.6 plus Red Hat fairy dust. Sorry for not checking upstream beforehand.

#2738 pretty much does the same as my #3100: it enhances the kernel and initrd configuration lines with what is in a variable: my PXE_SCHEME in #3100 demands a trailing slash, PXE_HTTP_URL in #2738 gets the slash from the code.

Also, #2738 enhances the GRUB menu with additional options (rear-automatic-http, rear-unattended-http and rear-http) while #3100 changes the existing options.

Personally I do not tend to enhancing the menu: PXE_TFTP_URL either points to the TFTP server download directory or the HTTP server download directory (assuming they are not the same). So with #2738 half the options won't work and the other half will work. You can't say from looking because you don't know where kernel and initrd have been uploaded. With #3100 there are no wrong choices (assuming PXE_TFTP_URL was set correctly).

#2738 did not make an attempt to download PXE_MESSAGE from the correct server and #3100 is a bit better regarding documentation in default.conf.

I would change my PR to use PXE_HTTP_URL (without trailing slash) instead of PXE_SCHEME. This has been discussed before). Also I would change it to undo the menu enhancement and instead modify the existing kernel/initrd paths. That would resurrect PXE_MESSAGE too. This would render it into a non-breaking change.

I would be interested in what @Flunkyball might have to say.

Flunkyball commented at 2023-12-07 20:45:¶

Good to see another fellow using PXE ;-)

I would change my PR to use PXE_HTTP_URL (without trailing slash) instead of PXE_SCHEME. This has been discussed before). Also I would change it to undo the menu enhancement and instead modify the existing kernel/initrd paths. That would resurrect PXE_MESSAGE too. This would render it into a non-breaking change.

This was actually also my first approach. The reason why I chose to add the additional option in the boot menu was due to the fact that there are still many systems around with a bios that does not support booting from HTTP.
With your configuration I have to choose exclusively HTTP on a rear config level loosing any chance to boot an older system.

So with https://github.com/rear/rear/pull/2738 half the options won't work and the other half will work. You can't say from looking because you don't know where kernel and initrd have been uploaded. With https://github.com/rear/rear/pull/3100 there are no wrong choices (assuming PXE_TFTP_URL was set correctly).

In my environment for the reason described before PXE is provided via HTTP and TFTP. Therefore both boot options are working fine.

In the end you could extend https://github.com/rear/rear/pull/3100 with an option to choose whether to create an additional grub option for TFTP. I did it the other way round.

robertdahlem commented at 2023-12-08 07:56:¶

@Flunkyball

This was actually also my first approach. The reason why I chose to add the additional option in the boot menu was due to the fact that there are still many systems around with a bios that does not support booting from HTTP.

I would like to understand this better: the difference whether a system can download kernel and initrd from my understanding does not come from the BIOS. The BIOS only downloads the "boot file" (typically pxelinux.0) by using TFTP. Even the pxelinux.cfg files are downloaded by TFTP (because there is no configuration yet saying otherwise).

Only when the configuration file has been read something else (kernel/initrd) can be downloaded by HTTP. And that is possible only if you have a pxelinux.0 which understands HTTP (i.e. by copying lpxelinux.0 over pxelinux.0 on the TFTP server or by reuesting lpxelinux.0 as "boot file"). But I did not yet see a dependency to a specific BIOS function for HTTP.

Could you please explain how you came to the conclusion that there is a dependency in the BIOS for HTTP downloads?

robertdahlem commented at 2023-12-08 08:16:¶

@Flunkyball

In my environment for the reason described before PXE is provided via HTTP and TFTP. Therefore both boot options are working fine.

I have another question: where do you upload kernel/initrd to? To the TFTP server or to the HTTP server? Or do they use the same directory? In this case: how do you handle the SELinux implications (tftpdir_rw_t vs. httpd_sys_content_t)?

Flunkyball commented at 2023-12-08 14:59:¶

I would like to understand this better: the difference whether a system can download kernel and initrd from my understanding does not come from the BIOS. The BIOS only downloads the "boot file" (typically pxelinux.0) by using TFTP. Even the pxelinux.cfg files are downloaded by TFTP (because there is no configuration yet saying otherwise).

Only when the configuration file has been read something else (kernel/initrd) can be downloaded by HTTP. And that is possible only if you have a pxelinux.0 which understands HTTP (i.e. by copying lpxelinux.0 over pxelinux.0 on the TFTP server or by reuesting lpxelinux.0 as "boot file"). But I did not yet see a dependency to a specific BIOS function for HTTP.

Could you please explain how you came to the conclusion that there is a dependency in the BIOS for HTTP downloads?

The PXE boot mechanism is a BIOS feature and therefore comes in a certain release version included with the BIOS.
Original PXE was not HTTP capable, this was introduced later. There is e.g. a free PXE implementation https://ipxe.org/ offering a slim chance to flash the rom of your network adapter or mainboard with an http compatible PXE version.

Otherwise no matter what initial information via TFTP is provided nothing will be downloaded.

robertdahlem commented at 2023-12-19 14:18:¶

@jsmeix

What the most reasonable way is how to implement HTTP support for PXE must be addressed before merging.

Absolutely. I'm involved in another project at the moment but will come back to this before end of month.

I'm pretty unhappy with the choice of variable name (PXE_HTTP_URL) in #2738: The well known PXE_TFTP_URL points to an upload path while the new PXE_HTTP_URL (as implemented in #2738) points to a download path. I consider that confusing.

How do you think about an incompatible change? This would be PXE_HTTP_URL pointing to another upload path (to the HTTP servers directory, the default being what PXE_TFTP_URL points to). In this case we still need an HTTP download path, which I (here) configured in PXE_SCHEME. How about something like PXE_HTTP_DOWNLOAD?

Also I think offering TFTP and HTTP both (as in #2738) is reasonable. This should stay.

I disagree with what has been written about PXE BIOSes being able to do HTTP downloads. From my point of view this is not backed by the Wikipedia PXE article. But actually that doesn't matter for our purposes. Either the client actually has this HTTP enabled PXE BIOS or one uses lpxelinux.0 instead of pxelinux.0.

Flunkyball commented at 2023-12-19 18:43:¶

How do you think about an incompatible change? This would be PXE_HTTP_URL pointing to another upload path (to the HTTP servers directory, the default being what PXE_TFTP_URL points to). In this case we still need an HTTP download path, which I (here) configured in PXE_SCHEME. How about something like PXE_HTTP_DOWNLOAD?

I do not see in what way this is confusing as the HTTP path is for download only. In what scenario would this be an upload path ?

I disagree with what has been written about PXE BIOSes being able to do HTTP downloads. From my point of view this is not backed by the Wikipedia PXE article. But actually that doesn't matter for our purposes. Either the client actually has this HTTP enabled PXE BIOS or one uses lpxelinux.0 instead of pxelinux.0.

The wikipedia article does not mention the HTTP path since it was not originally part of PXE. However, as you mentioned, not important for the solution if we keep both options.

robertdahlem commented at 2023-12-19 19:08:¶

I do not see in what way this is confusing as the HTTP path is for download only. In what scenario would this be an upload path ?

Don't you think that if you have an existing variable PXE_TFTP_URL, which points to an upload directory for TFTP that it is confusing to invent a variable PXE_HTTP_URL, which does not point to an upload directory for HTTP?

Flunkyball commented at 2023-12-19 19:16:¶

I do not see in what way this is confusing as the HTTP path is for download only. In what scenario would this be an upload path ?

Don't you think that if you have an existing variable PXE_TFTP_URL, which points to an upload directory for TFTP that it is confusing to invent a variable PXE_HTTP_URL, which does not point to an upload directory for HTTP?

No because PXE is by default a one way street in terms of HTTP and therefore does not confuse me.
So in the end you propose a breaking change to resolve your personal confusion ?

https://github.com/rear/rear/pull/2738 has been introduced 2 years ago. Probably some people used the feature since successfully and are forced to change their configuration after that, most likely finding out about it the hard way.

robertdahlem commented at 2023-12-19 19:31:¶

I'm pretty unhappy with the choice of variable name (PXE_HTTP_URL) in #2738: The well known PXE_TFTP_URL points to an upload path while the new PXE_HTTP_URL (as implemented in #2738) points to a download path. I consider that confusing.

@jsmeix Do you have an opinion about this?

jsmeix commented at 2023-12-20 10:24:¶

Offhandedly and as non PXE user I can only speak
in general about user config variable names:

In general I think backward compatibility
has higher value in practice for our users
than backward incompatibe improvements, cf.
https://github.com/rear/rear/wiki/Coding-Style#maintain-backward-compatibility
as far as possible with reasonable effort for us.
For example when a real bug cannot be fixed without
backward incompatibe changes then it is better
to have the bug fixed than to keep things wrong.

In particular regarding user config variable names:
Basically we cannot change existing user config variable names
to avoid regressions for users who use existing config variable names
regardless how awkward existing config variable names are, cf.
https://github.com/rear/rear/blob/rear-2.7/usr/share/rear/conf/default.conf#L907
which points to
https://github.com/rear/rear/pull/2293#issuecomment-564439509

When we have unclear existing config variable names
we can explain things properly in default.conf and
inform the user about possible misunderstandings
even if that leads to longer explanations
which could be sometimes even long-winded.

In contrast to insufficient information in default.conf
long-winded explanations are not a serious problem
because that can be easily improved by contributors
who can contribute more helpful explanation texts.

By the way:
Compare traditional Unix command names,
for example 'time' versus 'date'
and 'cancel' versus 'kill' ;-)

robertdahlem commented at 2023-12-20 13:56:¶

@jsmeix

In this case (backward compatibility has higher value than fixing awkward variable names) I propose something like PXE_HTTP_UPLOAD_URL, which MAY be set and then points to a second/different upload location. I would offer this because it is perfectly possible to need to upload the files for TFTP to another server than the files for HTTP, especially in enterprise environments, where you can't just let both servers point to the same directory: SELinux prevents you from doing so.

Then there is the point of the display action in PXE_CONFIG_FILE. It is ignored by #2738 (probably because this PR is intended for an environment where TFTP and HTTP server use the same directory). I propose to let it point to $PXE_HTTP_URL if this variable is set is set. In this case you download it from the TFTP server unless you explicitly state there is an HTTP server by setting PXE_HTTP_URL.

jsmeix commented at 2023-12-22 13:23:¶

As non PXE user I cannot help much with PXE specific things
but I try my best (and bear with me when I write nonsense):

Ideally when we need to distinguish between
the protocols TFTP versus HTTP and distinguish between
upload (i.e. wherefrom ReaR runs up to the PXE server) and
download (i.e. from the PXE server down to the booting computer)
the variable names should contain
both the protocol plus
UPLOAD and DOWNLOAD like

PXE_TFTP_UPLOAD_URL
PXE_TFTP_DOWNLOAD_URL
PXE_HTTP_UPLOAD_URL
PXE_HTTP_DOWNLOAD_URL

Unfortunately whe have already
PXE_TFTP_URL for upload and
PXE_HTTP_URL for download
which is confusing.
I think it is only confusing when both protocols are used
because when only one protocol is used for upload and download
then
either PXE_TFTP_URL for upload and download with TFTP
or PXE_HTTP_URL for upload and download with HTTP
would be consistent.

But when both the protocol and the direction (up vs down)
can be freely combined then specific variables are needed
so the current
PXE_TFTP_URL for upload with TFTP and
PXE_HTTP_URL for download with HTTP
cannot work
because a user cannot specify the opposite
PXE_TFTP_URL for download with TFTP and
PXE_HTTP_URL for upload with HTTP
i.e. the current variable naming is a bug
(when protocol and direction can be freely combined).

If we have this bug in current ReaR we have to fix it,
but not this year ;-)

@robertdahlem @Flunkyball
I wish you Merry Christmas and a happy New Year!

Flunkyball commented at 2023-12-22 13:48:¶

PXE_TFTP_UPLOAD_URL
PXE_TFTP_DOWNLOAD_URL
PXE_HTTP_UPLOAD_URL
PXE_HTTP_DOWNLOAD_URL
Unfortunately whe have already PXE_TFTP_URL for upload and PXE_HTTP_URL for download which is confusing. I think it is only confusing when both protocols are used because when only one protocol is used for upload and download then either PXE_TFTP_URL for upload and download with TFTP or PXE_HTTP_URL for upload and download with HTTP would be consistent.

But when both the protocol and the direction (up vs down) can be freely combined then specific variables are needed so the current PXE_TFTP_URL for upload with TFTP and PXE_HTTP_URL for download with HTTP cannot work because a user cannot specify the opposite PXE_TFTP_URL for download with TFTP and PXE_HTTP_URL for upload with HTTP i.e. the current variable naming is a bug (when protocol and direction can be freely combined).

Well... that is partly correct.
The thing is, that PXE originally had only TFTP for up and download and later added HTTP for download only.
-> PXE_TFTP is Up and Download where HTTP is Download only if it is used since it is optional.

Things get even messier if you look at my current rear config for PXE

PXE_TFTP_IP=192.xx.xx.xx
PXE_TFTP_URL="sshfs://pxe@pxe-ssh/tftp"
PXE_CONFIG_URL="sshfs://pxe@pxe-ssh/tftp/pxelinux.cfg"
PXE_HTTP_URL="http://$PXE_TFTP_IP:8000"

So in my case I even use SFTP for uploading to the PXE server through the TFTP_URL variable... .
In the end with https://github.com/rear/rear/pull/2738 this did not bother me because the same upload target does later on provide the TFTP download which makes it plausible for me.
However the HTTP_URL is added as additional download option thus the explicit additional variable.

In order to come clean you would need to rename "PXE_TFTP_URL" to "PXE_UPLOAD_URL" to be used with whatever protocol you want for upload and to have for each additional download option a seperate variable "PXE_PROTXX_DOWNLOAD"

@jsmeix @robertdahlem
Wish you Merry Christmas and a Happy New Year, too !

robertdahlem commented at 2023-12-29 15:51:¶

@jsmeix

PXE_TFTP_UPLOAD_URL
PXE_TFTP_DOWNLOAD_URL
PXE_HTTP_UPLOAD_URL
PXE_HTTP_DOWNLOAD_URL

Now that you introduced a complete new name space things got very easy: we can deprecate the old variable names, but still allow to use them. In this case warnings are generated, but the values of the old variables get assigned to the new variables. No breaking changes until someone decides to delete the deprecated variables (maybe in 3.0?).

I'll submit a change to introduce:

PXE_TFTP_URL -> PXE_TFTP_UPLOAD_URL
                PXE_TFTP_DOWNLOAD_URL isn't needed because it is the default and the value would always be ""
                PXE_HTTP_UPLOAD_URL (new variable)
PXE_HTTP_URL -> PXE_HTTP_DOWNLOAD_URL

robertdahlem commented at 2023-12-29 15:54:¶

Obviously I f*cked things up and now commits from other people show up between my commits. I might need a little help here.

robertdahlem commented at 2024-01-01 09:39:¶

Obviously I f*cked things up and now commits from other people show up between my commits. I might need a little help here.

I seem to have been able to repair my f*ckup:

git checkout master
git merge
git checkout http-download
git reset --hard master
re-apply patches (vi/git add/git commit)
git push --force

I know see my intended commits here.

jsmeix commented at 2024-01-02 12:12:¶

@robertdahlem
when I look at your current changes via the GitHub web frontend
https://github.com/rear/rear/pull/3100/files
it shows that there is still PXE_SCHEME implemented.
I thought PXE_HTTP_DOWNLOAD_URL is meant to replace PXE_SCHEME?

Regarding "commits from other people show up between my commits":
This happened also to me some longer time ago.
I don't remember the details and I am not a git expert
to imagine what goes on behind the surface in such cases.
I guess it may happen when one rebases his own branch on master
that then the newest commits of others from the master branch appear
intermixed with one's own commits (at least via the GitHub web frontend)
which is confusing because I want my branch to contain only my commits.

pcahyna commented at 2024-01-02 12:51:¶

Regarding "commits from other people show up between my commits":
This happened also to me some longer time ago.
I don't remember the details and I am not a git expert
to imagine what goes on behind the surface in such cases.
I guess it may happen when one rebases his own branch on master
that then the newest commits of others from the master branch appear
intermixed with one's own commits (at least via the GitHub web frontend)
which is confusing because I want my branch to contain only my commits.

No, IMO it happened because of merging the branch instead of rebasing it: see merge commits https://github.com/rear/rear/commit/c7885a854e19b1bbc4187ebe389fdeeb296ddf50 and https://github.com/rear/rear/commit/17c34baf781c0c6caf20193828d9c9659a787a7c . Rebase would have been fine.

@robertdahlem instead of manually re-applying desired patches, you could use git rebase --interactive or git cherry-pick.

robertdahlem commented at 2024-01-03 17:44:¶

@jsmeix

@robertdahlem when I look at your current changes via the GitHub web frontend https://github.com/rear/rear/pull/3100/files it shows that there is still PXE_SCHEME implemented. I thought PXE_HTTP_DOWNLOAD_URL is meant to replace PXE_SCHEME?

You are right. This was a remnant from previous code. I eliminated that now.

Do you think we can merge that now? @Flunkyball ?

@pcahyna
My workflow after having made my changes locally in mybranch seems stable now:
In my GitHub and my fork: Branch master > Sync fork > Update branch
Locally:

# git checkout master
# git pull
# git checkout mybranch
# git merge master
# git push

pcahyna commented at 2024-01-03 18:16:¶

@robertdahlem there is still a needless merge commit in the history now: https://github.com/rear/rear/pull/3100/commits/11f4e0284c7b757444de94feecec56295b013114
Would it work for you to use

# git rebase master
# git push -f

instead of

# git merge master
# git push

?
Alternatively, since you changes do not seem to conflict with any of the new development code, you could avoid merging or rebasing your branch entirely (this can not be relied on in general, of course).

robertdahlem commented at 2024-01-03 18:51:¶

@pcahyna

@robertdahlem there is still a needless merge commit in the history now: 11f4e02

Yes, I see. But it doesn't show up in the files changed by this PR. Also it doesn't show up in git diff master http-download. So I thought it is fine.

Would it work for you to use
# git rebase master
# git push -f

It might. It did not try that. I am still confused about the rebase stuff. Also rebase and force push drove me into the f*ckup. Additionally I think that the merge commit is not needless. It proves that my patches don't conflict with master.

The history of all that is:

I push a PR
Discussion ensues, further local development happens
Meanwhile people commit other stuff to master
To prove that my locally developed stuff does not conflict I need to merge my branch with master before pushing. Also I shy away from doing anything with --force.
Or don't I merge? What do people do at this point (aka "another push to a branch while master has moved on")?

? Alternatively, since you changes do not seem to conflict with any of the new development code, you could avoid merging or rebasing your branch entirely (this can not be relied on in general, of course).

Exactly. There must be some workflow that makes everybody happy.

robertdahlem commented at 2024-01-04 09:53:¶

@pcahyna

Exactly. There must be some workflow that makes everybody happy.

Ok. I tried (in another project):

# git rebase master
# git push --force

and after merging that I see my commits and one last commit Merge pull request #2 from robertdahlem/mybranch.

I am still slightly frightened about --force but if that is the workflow it will be ok for me.

@jsmeix Do I need to re-issue my PR or can we go on with the way it is yet?

jsmeix commented at 2024-01-04 10:33:¶

@robertdahlem
regarding your

I think that the merge commit is not needless.
It proves that my patches don't conflict with master.

Normally I don't update my branches with master.
I only do the changes that I need in my branches
and finally I merge those changes into master.
Only if there are merge conflicts I adapt the changes in my branch.
But normally there are no merge conflicts even if
common files (like default.conf) were changed in master
because usually changes in master are for different parts
in a file so that the git three-way merge can still merge
my changes from my branch without merge conflicts.

When I am logged in at GitHub I can see whether or not
the changes in a pull request can be merged without conflicts.
E.g. in this pull request here the GitHub web frontend shows

This branch has no conflicts with the base branch
Merging can be performed automatically.

This is not visible unless one is logged in at GitHub.
But I don't know if this is visible to every user who is
logged in at GitHub or if this is only visible to users
who have the right to merge pull requests (like me).

So bottom line from my point of view:
Don't worry in advance about possible merge conflicts.

jsmeix commented at 2024-01-04 11:35:¶

@pcahyna
I would like your opinion regarding the new
usr/share/rear/init/default/002_pxe_sanity_checks.sh

My current thoughts regarding the new
usr/share/rear/init/default/002_pxe_sanity_checks.sh

Up to now we do not have any _sanity_ named script.
But we have usr/share/rear/init/default/010_EFISTUB_check.sh
so usr/share/rear/init/default/020_PXE_check.sh
could be a better name?

In general I wonder if feature specific check scripts
should be always run in the general 'init' stage?
Wouldn't be a feature specific sub-directory better?
E.g. something like
usr/share/rear/prep/NETFS/default/050_check_NETFS_requirements.sh
with its symbolic link
usr/share/rear/verify/NETFS/default/050_check_NETFS_requirements.sh
On the other hand such symbolic links look awkward
when a check is basically always needed (where "basically always"
means it is in particular needed during 'mkrescue' and 'recover').
But the general 'init' stage is always run
so for generic checks - even when feature specific - the
general 'init' stage could be the best place in practice.

I think the messages should be shorter,
in particular without the word "warning", cf.
https://schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html
and without separator lines -------- that we normally don't use.
E.g. for comparison see the "deprecated" messages in
usr/share/rear/lib/borg-functions.sh
(that is rather old code before LogPrintError was introduced)

pcahyna commented at 2024-01-04 16:05:¶

@pcahyna

@robertdahlem there is still a needless merge commit in the history now: 11f4e02

Yes, I see. But it doesn't show up in the files changed by this PR. Also it doesn't show up in git diff master http-download. So I thought it is fine.

It is not a big problem for sure, it just complicates and clutters the history a bit for those that are going to look at it later. Also, I suspected that your 'commits from other people show up between my commits' problem was caused by such merge commits, where GitHub got confused by the too complicated history. But as the problem does not appear anymore, I am not sure. Maybe it was something else.

Would it work for you to use
# git rebase master
# git push -f
It might. It did not try that. I am still confused about the rebase stuff. Also rebase and force push drove me into the f*ckup. Additionally I think that the merge commit is not needless. It proves that my patches don't conflict with master.

The history of all that is:
* I push a PR

* Discussion ensues, further local development happens

* Meanwhile people commit other stuff to master

* To prove that my locally developed stuff does not conflict I need to merge my branch with master before pushing. Also I shy away from doing anything with `--force`.

* Or don't I merge? What do people do at this point (aka "another push to a branch while master has moved on")?

I admit I am usually sloppy with that, just like @jsmeix. I keep my branch not updated and keep pushing to it, despite that checking that the branch does not conflict with new changes as you do is for sure the right thing to do. Merely merging with the updated master branch and pushing the merge commit does not itself prove it, though. It only proves that a merge is possible (i.e. that there is no textual conflict), not that the merge will actually work. There could be a semantic conflict. Consider using a shared function which in the meantime gets renamed. Since the rename occurs in a different file than you are editing, it appears to merge fine, but the result will not work. So, it is not enough to merge the branches, you need to re-test the merge result. Merging the branches by itself does not prove much, it is good as the first step for testing the updated result. Rebasing on top of the current master does have the same effect though. If you don't like --force, merging is probably the best option. Note though that force pushes should show in the web UI, see

@robertdahlem force-pushed the http-download branch from 6725d98 to 104858f

in the PR history. This should allow you to go back when the force push messes up something.

If you are just interested in whether the merge is possible, the GitHub UI should tell you without attempting the merge itself, as @jsmeix mentioned. For example see https://github.com/rear/rear/pull/3041. Do you see the 'This branch has conflicts that must be resolved' message at the bottom?

Anyway, all this is not very important. Thank you a lot for you contribution, I will switch to reviewing the content of it instead of nitpicking about the Git process.

pcahyna commented at 2024-01-04 16:07:¶

in particular without the word "warning"

I would not be afraid of it. IMO, deprecation messages are a good use of the word 'warning'. It is something that is not a problem now, but it will be a problem in the future.

robertdahlem commented at 2024-01-04 16:20:¶

@jsmeix

I think the messages should be shorter,
in particular without the word "warning", cf.
https://schlomo.schapiro.org/2015/04/warning-is-waste-of-my-time.html

Schlomo is a man of strong opinions. :-)

I dare to disagree with him in this particular case, especially where he states "ERROR means that I as a human should take action. Everything else is irrelevant for me."

Most of these warnings end with "Please fix your configuration" which is pretty much the same as "human, you should take action". No bad things will happen (for now) when you ignore them, so they are not ERROR. Bad things WILL happen when sometime in the future "deprecated" turns into "gone", that makes them relevant.

You should see this, you should act on it, but they are not ERROR.

As for the name and position of 002_pxe_sanity_checks.sh in the directory structure: I just used what looked practical.

pcahyna commented at 2024-01-04 16:46:¶

Up to now we do not have any _sanity_ named script. But we have usr/share/rear/init/default/010_EFISTUB_check.sh so usr/share/rear/init/default/020_PXE_check.sh could be a better name?

There are many other scripts with check in their name in the prep stage and they also check the consistency of the variables, so check seems good choice indeed. For another possibility see https://github.com/rear/rear/blob/master/usr/share/rear/prep/default/020_translate_url.sh (it also contains many deprecation messages that could serve as another inspiration).

In general I wonder if feature specific check scripts should be always run in the general 'init' stage? Wouldn't be a feature specific sub-directory better? E.g. something like usr/share/rear/prep/NETFS/default/050_check_NETFS_requirements.sh with its symbolic link usr/share/rear/verify/NETFS/default/050_check_NETFS_requirements.sh On the other hand such symbolic links look awkward when a check is basically always needed (where "basically always" means it is in particular needed during 'mkrescue' and 'recover'). But the general 'init' stage is always run so for generic checks - even when feature specific - the general 'init' stage could be the best place in practice.

That's the point - I don't think that this needs to run during recovery (and bother the user with deprecation messages), right? So it could be just under prep, without any symlinks. Moreover, it probably does not need to be under default to run for every OUTPUT, PXE directory should be enough. So I would put it under prep/PXE/default.

robertdahlem commented at 2024-01-05 07:49:¶

@pcahyna @jsmeix

So it could be just under prep, without any symlinks. Moreover, it probably does not need to be under default to run for every OUTPUT, PXE directory should be enough. So I would put it under prep/PXE/default.

Done: 9a9e9df7a60182a38fef79e69ded25084eb4c1d2

jsmeix commented at 2024-01-05 08:13:¶

@Flunkyball @pcahyna @rear/contributors
please have a look here as time permits
and ideally test it if possible for you.
Thank you in advance!

jsmeix commented at 2024-01-05 08:23:¶

Currently the GitHub web frontend for this pull request shows

1 workflow awaiting approval
This workflow requires approval from a maintainer.
4 neutral, and 11 successful checks
[Approve and run]

How could "a maintainer" act on this when it does not tell him
what specific workflow "This workflow" actually is?

The generic GitHub information
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks
only tells vaguely why that approval is needed

Although workflows from forks do not have
access to sensitive data such as secrets,
they can be an annoyance for maintainers
if they are modified for abusive purposes.

But I cannot know if and how "This workflow"
was modified for abusive purposes
and what abusive purposes are possible here
so how could one make a decision in this specific case?

@pcahyna
do you perhaps know what "This workflow" actually is
and if and how "This workflow" could have been modified
for abusive purposes and what abusive purposes
might be possible with "This workflow"?

robertdahlem commented at 2024-01-05 09:42:¶

@jsmeix

But I cannot know if and how "This workflow"
was modified for abusive purposes
and what abusive purposes are possible here
so how could one make a decision in this specific case?

It specifically addresses the .github/workflow directory which probably does automated quality assurance stuff. Because it is automated it could clog resources or overwrite stuff, so there might be dragons. As I made no changes in this directory I think you can switch to YOLO mode and hit "Approve and run". :-)

Most changes in .github/workflow were made by @schlomo and @gdha. Maybe they can sched some light?

pcahyna commented at 2024-01-05 10:18:¶

@jsmeix

Currently the GitHub web frontend for this pull request shows
1 workflow awaiting approval
This workflow requires approval from a maintainer.
4 neutral, and 11 successful checks
[Approve and run]
How could "a maintainer" act on this when it does not tell him what specific workflow "This workflow" actually is?

The generic GitHub information https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks only tells vaguely why that approval is needed
Although workflows from forks do not have
access to sensitive data such as secrets,
they can be an annoyance for maintainers
if they are modified for abusive purposes.
But I cannot know if and how "This workflow" was modified for abusive purposes and what abusive purposes are possible here so how could one make a decision in this specific case?

@pcahyna do you perhaps know what "This workflow" actually is and if and how "This workflow" could have been modified for abusive purposes and what abusive purposes might be possible with "This workflow"?

mentioning "This workflow" without telling which is really very annoying. I go to the "Files changed" section and check that no workflow files (under .github) were modified, the changes should then be safe.

pcahyna commented at 2024-01-05 10:19:¶

In practice, when I approve the workflow, the only thing that runs is Differential ShellCheck.

pcahyna commented at 2024-01-05 10:33:¶

@robertdahlem
I see that you are changing some variables to lowercase, because they are local to the script. Which is good (assuming that they are really not used anywhere else - I have not checked). But I think that you should also declare them as local then. https://github.com/rear/rear/wiki/Coding-Style#variables .

robertdahlem commented at 2024-01-05 13:54:¶

@pchayna

@robertdahlem I see that you are changing some variables to lowercase, because they are local to the script. Which is good (assuming that they are really not used anywhere else - I have not checked).

I have. :-)

But I think that you should also declare them as local then. https://github.com/rear/rear/wiki/Coding-Style#variables .

Fixed: 405d9fa9da2229299299c84a1bae7437383416ed

robertdahlem commented at 2024-01-05 14:18:¶

@schlomo

I put in a bunch of fixes around the use of positive logic and quoting.

Commited: a2e383495c83eef12140845fa008aa54a19aa41d

Do you see a way to merge both copy scripts (for TFTP and for HTTP) into one? They seem to be doing nearly the same.

I'm not too happy with implementing that idea. They have more different than common lines and adding logic to tell these apart would only complicate things. I'm not actually opposed to the idea but would leave that as exercise to interested parties.

Another thought that keeps me wondering is the following: If putting the boot files onto a TFTP or HTTP server requires file system access, then why do we need to differentiate so much between TFTP and HTTP?

In an SELinux environment you can't (easily) resort to one common directory. /var/lib/tftpboot has tftpdir_rw_t and can't be accessed by httpd. /var/www/whatever has httpd_sys_content_t and can't be accessed by tftpd.

If you use an environment where you can mix them, you can configure PXE_HTTP_UPLOAD_URL="$PXE_TFTP_UPLOAD_URL" and be done with one directory. But I wouldn't default to this because it triggers SELinux debugging and that is not everybody's cup of tea. Even die hard RHEL guys like to maneuver around that. :-)

robertdahlem commented at 2024-01-08 23:23:¶

@jsmeix I have lost track a little bit ... are all discussion points, suggested changes etc. resolved? Or do you still need me to fix something before we can merge?

jsmeix commented at 2024-01-09 09:45:¶

@robertdahlem
when you tested the current final state
and this state works well for you,
then I would like to merge it "bona fide"
with one or two days delay so that others
like @Flunkyball could also have a look
and ideally test it in their environment.

Currently we are in ReaR 2.8 development phase
and there is not yet a ReaR 2.8 release date planned
(i.e there is plenty of time until ReaR 2.8 release)
so others who are interested could test things
after it was merged and if issues appear
we have time to fix them properly.

By the way:
Since a long time it is on my private todo list
to learn PXE booting but I never found time for it.
Perhaps this issue will finally let me do that.
But no promises. It all depends on whether or not
other stuff takes attention with higher priority,
in particular issues of SUSE customers who pay my salary.

jsmeix commented at 2024-01-12 12:51:¶

@robertdahlem
did you test the current final state
and does this state work well for you?

robertdahlem commented at 2024-01-12 16:43:¶

@robertdahlem did you test the current final state and does this state work well for you?

@jsmeix I've been busy with other stuff, sorry. Paying customers, you know the drill. :-)

I've just built a current RPM and delivered it to my workplace. I have installed that RPM on several bare metal test machines. They are backupped with Rubrik (BACKUP=CDM/OUTPUT=PXE) and I have to wait until Monday when the next backup cycle is through. My colleagues will do a regression test with BACKUP=NETFS/OUTPUT=ISO.

I will report.

robertdahlem commented at 2024-01-17 12:09:¶

@jsmeix

I've just built a current RPM and delivered it to my workplace. I have installed that RPM on several bare metal test machines. They are backupped with Rubrik (BACKUP=CDM/OUTPUT=PXE) and I have to wait until Monday when the next backup cycle is through. My colleagues will do a regression test with BACKUP=NETFS/OUTPUT=ISO.

I can report that BACKUP=CDM/OUTPUT=PXE works with TFTP and HTTP download protocols. Also there have been no regressions with our standard BACKUP=NETFS/OUTPUT=ISO.

Colleagues now are eager to switch to BACKUP=NETFS/OUTPUT=PXE. :-)

So yes, we did test the final state and it worked well for us.

pcahyna commented at 2024-01-17 12:26:¶

@robertdahlem thanks for the contribution and the thorough testing! I think you should edit the PR description as you have changed PXE_SCHEME to something else. Otherwise the code now looks good to me.

jsmeix commented at 2024-01-17 12:26:¶

@rear/contributors @Flunkyball
I intend to merge it on Friday (19 Jan. 2024) afternoon
unless there are objections.

jsmeix commented at 2024-01-17 13:02:¶

I would like to add a comment
about the variable names
PXE_TFTP_UPLOAD_URL
PXE_HTTP_UPLOAD_URL
PXE_HTTP_DOWNLOAD_URL

The example in default.conf

#    PXE_TFTP_UPLOAD_URL=nfs://tftp-server/var/lib/tftpboot
#    PXE_HTTP_UPLOAD_URL=nfs://web-server/var/www/html
#    PXE_HTTP_DOWNLOAD_URL=http://web-server

shows that for the TFTP and HTTP *_UPLOAD_URL variables
the actually used protocol ('nfs' in this case)
does not match the protocol in the variable name
which might be confusing at first glance.

The reason is that actually the *_UPLOAD_URL variables
could be named with full explanatory names like
PXE_UPLOAD_URL_FOR_TFTP_DOWNLOAD
PXE_UPLOAD_URL_FOR_HTTP_DOWNLOAD
but I think such names are oververbose
so for me the shorter names are fine.

Perhaps it could be explained in default.conf
that also for PXE_TFTP_UPLOAD_URL and PXE_HTTP_UPLOAD_URL
"nfs or any other mountable scheme" must be used
(i.e. same as already for PXE_CONFIG_URL)
but I think the example in default.conf is good enough.

For the fun of it:
Listing variable names in default.conf with their length:

# grep -o '^[A-Z][A-Z][^=]*' usr/share/rear/conf/default.conf | awk '{ print length, $0 }' | sort -n

2 OS
4 ARCH
4 LIBS
...
37 AUTOSHRINK_DISK_SIZE_LIMIT_PERCENTAGE
39 NON_FATAL_BINARIES_WITH_MISSING_LIBRARY
43 AUTOINCREASE_DISK_SIZE_THRESHOLD_PERCENTAGE

So the winner of the "longest config variable name contest"
is me ;-)

robertdahlem commented at 2024-01-17 14:02:¶

@robertdahlem thanks for the contribution and the thorough testing! I think you should edit the PR description as you have changed PXE_SCHEME to something else. Otherwise the code now looks good to me.

@pcahyna Fixed.

robertdahlem commented at 2024-01-17 14:51:¶

Perhaps it could be explained in default.conf that also for PXE_TFTP_UPLOAD_URL and PXE_HTTP_UPLOAD_URL "nfs or any other mountable scheme" must be used (i.e. same as already for PXE_CONFIG_URL) but I think the example in default.conf is good enough.

@jsmeix
For PXE_HTTP_UPLOAD_URL it already says so:

# PXE_HTTP_UPLOAD_URL must be set and point to your
# web server "DocumentRoot" (httpd) or "root" (nginx) directory
# using nfs or any other mountable scheme.

For PXE_TFTP_UPLOAD_URL I added an explanation.

pcahyna commented at 2024-01-17 15:20:¶

@robertdahlem something bad has happened with your merge (with uptodate master) and push. What did you do?

robertdahlem commented at 2024-01-17 17:47:¶

@robertdahlem something bad has happened with your merge (with uptodate master) and push. What did you do?

@pcahyna
What exactly is "something bad"? My git log reads:

commit ccb9d9a9d95c19872abc9d416a34e9863cd0c72e
Merge: ebbad506 3e876e1f
Author: Robert Dahlem <robert.dahlem@gmx.net>
Date:   Wed Jan 17 15:50:15 2024 +0100

    add some explanation for PXE_TFTP_UPLOAD_URL

commit ebbad506e033962867c876e74f69a822ba1302f9
Author: Robert Dahlem <robert.dahlem@gmx.net>
Date:   Fri Jan 5 15:48:45 2024 +0100

    silently return if nothing to be done

and git diff ebbad506e033962867c876e74f69a822ba1302f9 ccb9d9a9d95c19872abc9d416a34e9863cd0c72e says:

index ef3a8a9b..a23a2ee6 100644
--- a/usr/share/rear/conf/default.conf
+++ b/usr/share/rear/conf/default.conf
@@ -1335,6 +1335,8 @@ PXE_TFTP_IP=
 # where should we put the TFTP files ? (legacy way)
 PXE_TFTP_PATH=/var/lib/rear/output
 # where should we put the TFTP files ? (URL style)
+# PXE_TFTP_UPLOAD_URL must be set and point to your
+# tftp server drectory using nfs or any other mountable scheme.
 #PXE_TFTP_URL: deprecated as of 2.8. Use PXE_TFTP_UPLOAD_URL
 PXE_TFTP_UPLOAD_URL=
 #

pcahyna commented at 2024-01-17 18:59:¶

@robertdahlem https://github.com/rear/rear/pull/3100/commits suddenly shows 29 commits and there are duplicates (like https://github.com/rear/rear/pull/3100/commits/d751b64c5523e78e306a75b606cfb777d109f170 and https://github.com/rear/rear/pull/3100/commits/655eb8d1bed9ca48784a354108c4c5980223b4c5 ) I will check out your branch and examine it locally.

pcahyna commented at 2024-01-17 19:05:¶

commit ccb9d9a9d95c19872abc9d416a34e9863cd0c72e
Merge: ebbad506 3e876e1f
Author: Robert Dahlem <robert.dahlem@gmx.net>
Date:   Wed Jan 17 15:50:15 2024 +0100

    add some explanation for PXE_TFTP_UPLOAD_URL

that's the problem - it should not be a merge, the parent commit 3e876e1fb0d88ceff24e6601d9955ad1b366c67b adds another copy of the history. Try gitk to visualize it.

robertdahlem commented at 2024-01-17 19:23:¶

@pcahyna

that's the problem - it should not be a merge, the parent commit 3e876e1 adds another copy of the history. Try gitk to visualize it.

Bloody git. :-( I synced in GitHub, then

git checkout master
git pull
git checkout http-download
git rebase master
git pull # I think here might have been a merge
vi ./usr/share/rear/conf/default.conf
git add ./usr/share/rear/conf/default.conf
gcm "add some explanation for PXE_TFTP_UPLOAD_URL"
git push

Anyway. The "files changed" tab looks good to me. Do you need me to do something?

pcahyna commented at 2024-01-17 19:56:¶

I synced in GitHub

Meaning, you did a merge it in the GitHub WebUI?

git pull # I think here might have been a merge

Yes. git pull = git fetch + git merge.

The rebased version (after git rebase master - rev. ebbad506e033962867c876e74f69a822ba1302f9 ) was already fine, so the git pull was not needed. Compare ebbad506e033962867c876e74f69a822ba1302f9 and 3e876e1fb0d88ceff24e6601d9955ad1b366c67b - they are identical.

gcm "add some explanation for PXE_TFTP_UPLOAD_URL"

what is "gcm" ? (Looks like git commit --amend)

Anyway. The "files changed" tab looks good to me. Do you need me to do something?

Please, in your http-download branch, do
git reset --hard ebbad506 - this will reset your branch to the revision after git rebase
git cherry-pick -m1 ccb9d9a9d95c19872abc9d416a34e9863cd0c72e - this will reapply your "add some explanation for PXE_TFTP_UPLOAD_URL" commit
git push --force

To verify that this procedure did not introduce any file changes, do git diff ccb9d9a9d95c19872abc9d416a34e9863cd0c72e - it should return nothing.

robertdahlem commented at 2024-01-18 08:40:¶

@pcahyna

I synced in GitHub
Meaning, you did a merge it in the GitHub WebUI?

I clicked on "Sync fork" in the WebUI.

The rebased version (after git rebase master - rev. ebbad50 ) was already fine, so the git pull was not needed.

I took an additional note in my documentation. "Do Never. Ever. use git pull when git rebase is sufficient". :-)
But how would I get commits which have been made remote (not in master, but in my branch)?

gcm "add some explanation for PXE_TFTP_UPLOAD_URL"
what is "gcm" ? (Looks like git commit --amend)

alias gcm='git commit -m'

Please, in your http-download branch, do git reset --hard ebbad506 - this will reset your branch to the revision after git rebase git cherry-pick -m1 ccb9d9a9d95c19872abc9d416a34e9863cd0c72e - this will reapply your "add some explanation for PXE_TFTP_UPLOAD_URL" commit git push --force

I did as you wrote. Is it ok now?

pcahyna commented at 2024-01-18 16:11:¶

@pcahyna

I synced in GitHub
Meaning, you did a merge it in the GitHub WebUI?

I clicked on "Sync fork" in the WebUI.

I see. I think it is preferable to avoid that and leave the branch descending from an older state. See jsmeix's https://github.com/rear/rear/pull/3100#issuecomment-1876866258 :

So bottom line from my point of view:
Don't worry in advance about possible merge conflicts.

The rebased version (after git rebase master - rev. ebbad50 ) was already fine, so the git pull was not needed.

I took an additional note in my documentation. "Do Never. Ever. use git pull when git rebase is sufficient". :-) But how would I get commits which have been made remote (not in master, but in my branch)?

Yes, to update your branch to the uptodate master branch one would use git rebase or git merge (performed implicitly by git pull) but not both.

Usually there would not be any commits made in your branch at the remote side. But here I see that you needed to get the additional commit that was created by accepting the review suggestions in the WebUI. For this, I would use "git pull" first, before I use anything that changes my local history (git rebase, git reset). Or for better control git fetch followed by git merge. This would merely get the new commit(s) without merging anything (to enforce this, add --ff-only to git merge). Only when the local branch is in sync with the remote version, rebase if you want, and then force-push. But I think it may be better to not worry about updating to the current master branch code, so I would even skip that (I only rarely rebase).

gcm "add some explanation for PXE_TFTP_UPLOAD_URL"
what is "gcm" ? (Looks like git commit --amend)

alias gcm='git commit -m'

Please, in your http-download branch, do git reset --hard ebbad506 - this will reset your branch to the revision after git rebase git cherry-pick -m1 ccb9d9a9d95c19872abc9d416a34e9863cd0c72e - this will reapply your "add some explanation for PXE_TFTP_UPLOAD_URL" commit git push --force

I did as you wrote. Is it ok now?

Yes, it is good now. I think you can click on "compare" in the message about force-push above and see that your force-push did not introduce any content changes. The commit history looks good now.

pcahyna commented at 2024-01-22 18:49:¶

@jsmeix ready to merge?

jsmeix commented at 2024-01-23 06:33:¶

@pcahyna
I was ready to merge since
https://github.com/rear/rear/pull/3100#issuecomment-1895679167
but then I waited until the subsequent things got fixed.
I think in
https://github.com/rear/rear/pull/3100#issuecomment-1898784463
I missed your final "it is good now".
So, yes it should be ready to merge now.
I will merge it today afternoon unless someone objects.

[Export of Github issue for rear/rear.]

#3100 PR open: OUTPUT=PXE: Support for downloading kernel and initrd by http¶

robertdahlem opened issue at 2023-12-05 15:40:¶

Pull Request Details:¶

jsmeix commented at 2023-12-06 13:58:¶

pcahyna commented at 2023-12-06 14:05:¶

robertdahlem commented at 2023-12-06 14:28:¶

pcahyna commented at 2023-12-06 14:44:¶

robertdahlem commented at 2023-12-06 15:03:¶

jsmeix commented at 2023-12-07 12:35:¶

robertdahlem commented at 2023-12-07 14:08:¶

Flunkyball commented at 2023-12-07 20:45:¶

robertdahlem commented at 2023-12-08 07:56:¶

robertdahlem commented at 2023-12-08 08:16:¶

Flunkyball commented at 2023-12-08 14:59:¶

robertdahlem commented at 2023-12-19 14:18:¶

Flunkyball commented at 2023-12-19 18:43:¶

robertdahlem commented at 2023-12-19 19:08:¶

Flunkyball commented at 2023-12-19 19:16:¶

robertdahlem commented at 2023-12-19 19:31:¶

jsmeix commented at 2023-12-20 10:24:¶

robertdahlem commented at 2023-12-20 13:56:¶

jsmeix commented at 2023-12-22 13:23:¶

Flunkyball commented at 2023-12-22 13:48:¶

robertdahlem commented at 2023-12-29 15:51:¶

robertdahlem commented at 2023-12-29 15:54:¶

robertdahlem commented at 2024-01-01 09:39:¶

jsmeix commented at 2024-01-02 12:12:¶

pcahyna commented at 2024-01-02 12:51:¶

robertdahlem commented at 2024-01-03 17:44:¶

pcahyna commented at 2024-01-03 18:16:¶

robertdahlem commented at 2024-01-03 18:51:¶

robertdahlem commented at 2024-01-04 09:53:¶

jsmeix commented at 2024-01-04 10:33:¶

jsmeix commented at 2024-01-04 11:35:¶

pcahyna commented at 2024-01-04 16:05:¶

pcahyna commented at 2024-01-04 16:07:¶

robertdahlem commented at 2024-01-04 16:20:¶

pcahyna commented at 2024-01-04 16:46:¶

robertdahlem commented at 2024-01-05 07:49:¶

jsmeix commented at 2024-01-05 08:13:¶

jsmeix commented at 2024-01-05 08:23:¶

robertdahlem commented at 2024-01-05 09:42:¶

pcahyna commented at 2024-01-05 10:18:¶

pcahyna commented at 2024-01-05 10:19:¶

pcahyna commented at 2024-01-05 10:33:¶

robertdahlem commented at 2024-01-05 13:54:¶

robertdahlem commented at 2024-01-05 14:18:¶

robertdahlem commented at 2024-01-08 23:23:¶

jsmeix commented at 2024-01-09 09:45:¶

jsmeix commented at 2024-01-12 12:51:¶

robertdahlem commented at 2024-01-12 16:43:¶

robertdahlem commented at 2024-01-17 12:09:¶

pcahyna commented at 2024-01-17 12:26:¶

jsmeix commented at 2024-01-17 12:26:¶

jsmeix commented at 2024-01-17 13:02:¶

robertdahlem commented at 2024-01-17 14:02:¶

robertdahlem commented at 2024-01-17 14:51:¶

pcahyna commented at 2024-01-17 15:20:¶

robertdahlem commented at 2024-01-17 17:47:¶

pcahyna commented at 2024-01-17 18:59:¶

pcahyna commented at 2024-01-17 19:05:¶

robertdahlem commented at 2024-01-17 19:23:¶

pcahyna commented at 2024-01-17 19:56:¶

robertdahlem commented at 2024-01-18 08:40:¶

pcahyna commented at 2024-01-18 16:11:¶

pcahyna commented at 2024-01-22 18:49:¶

jsmeix commented at 2024-01-23 06:33:¶

#3100 PR `open`: OUTPUT=PXE: Support for downloading kernel and initrd by http¶