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

[patch] Bump default MINOR and PATCH versions #794

Closed
wants to merge 1 commit into from

Conversation

egypcio
Copy link

@egypcio egypcio commented Nov 5, 2022

since 988d2c1 we certainly changed
MINOR and PATCH versions to reflect tag v1.21.1, but it didn't change the default version reflected on 'src/Makefile' properly.

this is certainly important for builds which are not depending on git
to compile the code (which, tbh ain't a real dependency for ipxe itself)

by applying this patch we fix that and from now on "iPXE/1.21.1+" will
be shown as our user agent instead of the current one:

192.168.123.103 - - [03/Nov/2022:19:13:44 +0000] "GET /ipxe/menu.ipxe
    HTTP/1.1" 200 2411 "-" "iPXE/1.0.0+"

Signed-off-by: Vinícius Zavam egypcio@FreeBSD.org

  since 988d2c1 we certainly changed
MINOR and PATCH versions to reflect tag v1.21.1, but it didn't change
the default version reflected on 'src/Makefile' properly.

  this is certainly important for builds which are not depending on git
to compile the code (which, tbh ain't a real dependency for ipxe itself)

  by applying this patch we fix that and from now on "iPXE/1.21.+" will
be shown as our user agent instead of the current one:

```
192.168.123.103 - - [03/Nov/2022:19:13:44 +0000] "GET /ipxe/menu.ipxe
    HTTP/1.1" 200 2411 "-" "iPXE/1.0.0+"
```

Signed-off by: Vinícius Zavam <egypcio@googlemail.com>
Copy link
Contributor

@stappersg stappersg left a comment

Choose a reason for hiding this comment

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

Looks good to me

( Cross reference to #679 )

@NiKiZe
Copy link
Contributor

NiKiZe commented Nov 5, 2022

This instead adds the dependency of updating code for new releases instead of only adding the tag.

I'm on the NAK and closing side on this. Will leave it open for now, but will be closing if this stays open without action or comment from @mcb30

@egypcio
Copy link
Author

egypcio commented Nov 5, 2022

thank you for your thoughts and feedback @stappersg and @NiKiZe; much appreciated.

one little thing I would like to raise if I may is that, IMHO, this small change on the src/Makefile would not interfere or block any future release process you might build using methods like GitHub actions (or GitLab pipelines, or something else you might chose to implement) - once you will be running the build on a machine (or container) that likely has git available already.

the motivation for me on applying this little change is to support builds like the FreeBSD port I maintain myself - something that builds iPXE in a jail without having git in it (and we certainly would love to just keep it this way).

@NiKiZe
Copy link
Contributor

NiKiZe commented Nov 5, 2022

The risk of introducing inconsistent versions.
With 1.0.0+ we now know the build does not use git version.
And if that is the case then there is no meaning of having 1.21 since it says very little about the real version which is the git commit id.

@habetsm-xilinx
Copy link
Contributor

Creating a tag without updating the version in the Makefile is a bug, which this patch will fix.
Having said that, @NiKiZe is also right that building iPXE without the git commit included means we cannot give good support.

From worse situation to best it's

  1. Version 1.0.0+ without a commit ID
  2. Version 1.0.0+ with a commit ID
  3. Version 1.21.1+ without a commit ID
  4. Version 1.21.1+ with a commit ID

The other aspect for debugging is the output of git describe. For example:

$ git describe --tags da491eaa
v1.20.1-171-gda491eaa

This reflects it is based on tag 1.20.1. If the version that iPXE shows at run time is inconsistent with that it seems like something is very wrong. Is the user running a different version than they are executing? Other confusion will ensue.
FYI: I use git describe to set the VERSION in the Makefile of some of my other projects. Again, building with git available is a good thing.

1 nit on the commit: do not use backticks in the description. It should be pure text, not a meta format.

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Dec 27, 2022
  while here, bump default MINOR and PATCH versions
  - ipxe/ipxe#794
@egypcio
Copy link
Author

egypcio commented Feb 11, 2023

ping

as you can see, this one was applied to our FreeBSD port already; is there any chance you would merge it in here?

@Neustradamus Neustradamus mentioned this pull request Mar 2, 2023
@NiKiZe
Copy link
Contributor

NiKiZe commented Mar 2, 2023

Official builds is available at boot.ipxe.org.

Official version number uses git master and the git hash as version number.

I have no say on the matter, just stating facts.
No arguing about it will change these facts.

@NiKiZe
Copy link
Contributor

NiKiZe commented May 30, 2023

Closing, as I don't see that this will be merged.
If these values are to be updated, it needs to be so in an automatic way, together with the git tag.

For now the official answer still is, use a full git checkout, with git, and build from git master, which is the code we expect everyone to run if reporting any bugs.

@NiKiZe NiKiZe closed this May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants