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

Eliminate git sha from version in case of release #1188

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AAndrisa
Copy link
Collaborator

@AAndrisa AAndrisa commented Aug 1, 2024

PR Description

In order to introduce the packages of libiio in a package repo the git sha was eliminated from patch value of the version variable and replaced with 0.
This flow is particular to a release build and is set by setting an environment variable called IS_RELEASE. If the variable is not initialized then the build will remain the same.

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have commented new code, particulary complex or unclear areas
  • I have checked that I did not intoduced new warnings or errors (CI output)
  • I have checked that components that use libiio did not get broken
  • I have updated the documentation accordingly (GitHub Pages, READMEs, etc)

For the PATCH value of the VERSION variable the git sha of the last
commit was used, but in case of a release we want to change that value
to 0, so it can be added in a package repository.

If we set an environment variable IS_RELEASE then the PATCH variable will
remain 0, otherwise will still be initialized with git sha.

Signed-off-by: Andreea Andrisan <[email protected]>
After switching to patch variable artifact check also needs to be
updated.

Signed-off-by: Andreea Andrisan <[email protected]>
In order to extract the git sha of the last commit from libiio the
directory where a volume of the project was created needs to be set as
safe, otherwise an error message will appear.

Signed-off-by: Andreea Andrisan <[email protected]>
In order to maintain the same length of the git sha during package
creation all the jobs must have the same parameters at repository checkout.

Signed-off-by: Andreea Andrisan <[email protected]>
@AAndrisa
Copy link
Collaborator Author

AAndrisa commented Aug 1, 2024

Should this changes be implemented also on libiio-v0 branch for possible releases from that branch?

@@ -296,7 +296,7 @@ static int start_iiod(const char *uri, const char *ffs_mountpoint,

IIO_INFO("Starting IIO Daemon version %u.%u.%s\n",
LIBIIO_VERSION_MAJOR, LIBIIO_VERSION_MINOR,
LIBIIO_VERSION_GIT);
LIBIIO_VERSION_PATCH);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a great idea - since it will not be set to "0" all the time - and users reporting errors will no longer know what version of iiod they are using - something from main, or a release. with the git version - everyone knew.

Copy link
Contributor

Choose a reason for hiding this comment

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

For development propose and using it in other CI pipelines (like libad9361/9166, iio-osc etc), it's ok to keep using git_sha as patch version. Those daily-created packages are not used so often by customers.
For released versions, we are not allowed to use it anymore - released packages are added now in ADI Linux Package Repository and they need to have a number as patch version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is a great idea - since it will not be set to "0" all the time - and users reporting errors will no longer know what version of iiod they are using - something from main, or a release. with the git version - everyone knew.

If the build will be from main the git sha will appear as expected.
version

Copy link
Contributor

Choose a reason for hiding this comment

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

Then this is even worse - since sometimes a variable called "patch" is a githash, and sometimes it's not.

@rgetz
Copy link
Contributor

rgetz commented Aug 3, 2024

Should this changes be implemented also on libiio-v0 branch for possible releases from that branch?

Absolutely not.

This is not a fix or a bug - so it doesn't go on the -v0 branch.

@rgetz
Copy link
Contributor

rgetz commented Aug 3, 2024

To do things properly - I think there are many more changes required (and to keep backwards compatibility).

For example - deep in the context xml - it's called : "version-git" - I think we should leave it like that.

Packaging in the package managers needs to be done by each distribution - while we should make it easy for them to refer to something as a patch number - I don't think we need to abandon using git tags. They are invaluable when doing remote debug.

@SRaus
Copy link
Contributor

SRaus commented Aug 5, 2024

To do things properly - I think there are many more changes required (and to keep backwards compatibility).

For example - deep in the context xml - it's called : "version-git" - I think we should leave it like that.

Packaging in the package managers needs to be done by each distribution - while we should make it easy for them to refer to something as a patch number - I don't think we need to abandon using git tags. They are invaluable when doing remote debug.

We do not abandon using git_sha. They will still be used for daily builds. But for releases will use .0 as patch version. In this case, there will be a release tag, bind to a commit, so debuggability will be the same. The reason behind is that packages will be added in ADI Package repository (for multiple distributions) and keeping git_sha in name will mess up versions.

@SRaus
Copy link
Contributor

SRaus commented Aug 5, 2024

The new guidelines recommends that for all general available packages to indicate its level of stability in its name, by using tags like *_dev - Development / *_a - Alpha / *_b - Beta / *_rc - Release Candidate etc. We have not applied them yet for lib-iio, but probably we'll add _dev for non-released ones and _rc whenever we start testing a release.

@SRaus
Copy link
Contributor

SRaus commented Aug 5, 2024

Should this changes be implemented also on libiio-v0 branch for possible releases from that branch?

Absolutely not.

This is not a fix or a bug - so it doesn't go on the -v0 branch.

There is in plan to create one more release for v0 - v0.26.0. In order to add it in a linux package repo, the guideline is to have patch version as a digit/number, and not git_sha.

@rgetz
Copy link
Contributor

rgetz commented Aug 5, 2024

Should this changes be implemented also on libiio-v0 branch for possible releases from that branch?

Absolutely not.
This is not a fix or a bug - so it doesn't go on the -v0 branch.

There is in plan to create one more release for v0 - v0.26.0. In order to add it in a linux package repo, the guideline is to have patch version as a digit/number, and not git_sha.

like I said - this is not a fix or a bug - please do not do that.

I have no idea what "guideline" you are talking about.

Semantic Versioning - has been out for a long time (Dec 14, 2009) - for libiio - we actively decided NOT to use it. It wasn't an accident or by mistake that we decided to use githash - we decided that would be the better/easier thing to do.

Even in terms of Sematic versioning - you increment MINOR version when you add functionality in a backward compatible manner. Since dropping githash would break things - that's the definition of not backwards compatible.

@rgetz
Copy link
Contributor

rgetz commented Aug 5, 2024

We do not abandon using git_sha. They will still be used for daily builds.

have a look at the current patch set - it will not be used for daily builds.

	IIO_INFO("Starting IIO Daemon version %u.%u.%s\n",
		 LIBIIO_VERSION_MAJOR, LIBIIO_VERSION_MINOR,
-		 LIBIIO_VERSION_GIT);
+		 LIBIIO_VERSION_PATCH);

changes it for all builds, including daily.

In case the build was triggered for a release purpose we don't want to
use the git sha in the version of libiio.

The new variable will be used in order to identify that is a build
dedicated to a release.

Signed-off-by: Andreea Andrisan <[email protected]>
@AAndrisa
Copy link
Collaborator Author

AAndrisa commented Aug 6, 2024

We do not abandon using git_sha. They will still be used for daily builds.

have a look at the current patch set - it will not be used for daily builds.

	IIO_INFO("Starting IIO Daemon version %u.%u.%s\n",
		 LIBIIO_VERSION_MAJOR, LIBIIO_VERSION_MINOR,
-		 LIBIIO_VERSION_GIT);
+		 LIBIIO_VERSION_PATCH);

changes it for all builds, including daily.

The patch for daily builds won't be 0 because the variable LIBIIO_VERSION_PATCH is initialized with LIBIIO_VERSION_GIT in case of daily build

if (NOT IS_RELEASE_ENV)

@nunojsa
Copy link
Contributor

nunojsa commented Aug 7, 2024

Here it goes my 2 cents on this...

like I said - this is not a fix or a bug - please do not do that.

I totally agree with this. Completely unnecessary IMO and just risking unnecessary trouble...

Semantic Versioning - has been out for a long time (Dec 14, 2009) - for libiio - we actively decided NOT to use it. It wasn't an accident or by mistake that we decided to use githash - we decided that would be the better/easier thing to do.

Even in terms of Sematic versioning - you increment MINOR version when you add functionality in a backward compatible manner. Since dropping githash would break things - that's the definition of not backwards compatible.

No so sure on the above. If we properly tag all our releases (of course including when moving minors), it should still be fairly straight for people to check git shas on both iiod and libiio. We could even still continue to save that info in some common header if necessary (or with some command argument).

As for breaking things, well libiio1 is moving it's major so in theory it's expected to break thinks (and at this point it does break if you try to rebuild it). Yes, we do have a compat layer (that's only for runtime) and if there's something we can do to not break things, the better. But I would not block a change like this because of that.

I mean, for users to make real use of libiio1, they will anyways have to rebuild and re-adapt theirs apps for the new API.

FWIW, I don't really agree with this idea of the major number thingy to break compatibility. If you want to complete refactor things just call it another name. So I would have moved libiio to libiio2 just to be explicit about it 😅

@rgetz also for my understanding... How is this breaking current users?

@rgetz
Copy link
Contributor

rgetz commented Aug 7, 2024

completely adding minor (and leaving githash as is) so it's:

Releases

  • in below, githash is there, populated properly, but not part of the package/file name.
  • 1.0.0-rc1 (release candidate one)
  • 1.0.0-rc2 (release candidate two)
  • 1.0.0 (official release)

Building from source (main or other)

  • 1.0.0-githash (daily builds, development builds, etc)

would make more sense to me.

Today we have:

_api __pure unsigned int iio_context_get_version_major(const struct iio_context *ctx);
__api __pure unsigned int iio_context_get_version_minor(const struct iio_context *ctx);
__api __pure const char * iio_context_get_version_tag(const struct iio_context *ctx);

leave those alone, and add

__api __pure unsigned int iio_context_get_version_patch(const struct iio_context *ctx);

would make alot more sense. the compat layer would ignore that, and still report 1.0-gittag for things.

@nunojsa
Copy link
Contributor

nunojsa commented Aug 7, 2024

That looks sane to me and it is what I would expect to see from a linux package... Having the sha in a dev/daily build is fairly common.

@@ -15,6 +15,7 @@ endif()

set(LIBIIO_VERSION_MAJOR ${PROJECT_VERSION_MAJOR})
set(LIBIIO_VERSION_MINOR ${PROJECT_VERSION_MINOR})
set(LIBIIO_VERSION_PATCH 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be using ${PROJECT_VERSION_PATCH} per normal/standard cmake semantics.

@rgetz
Copy link
Contributor

rgetz commented Aug 7, 2024

@rgetz also for my understanding... How is this breaking current users?

I'm pretty sure - there is an old version of libiio that assumes githash is always a null terminated 8 char string, and uses strcpy, or memcpy to move it around. putting "0" (null) there will cause it to look like a null terminated string, and will not print "0". or could cause undefined results - it wasn't until 2020 that we ran everything through enough analysis tools - and 2022 until I ran it through polyspace checker - which found issues like this...

@AAndrisa AAndrisa marked this pull request as draft September 2, 2024 12:45
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.

4 participants