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

version info on non-git builds & platforms with SSE support only? #1514

Closed
yarikoptic opened this issue Dec 13, 2018 · 13 comments
Closed

version info on non-git builds & platforms with SSE support only? #1514

yarikoptic opened this issue Dec 13, 2018 · 13 comments
Assignees
Milestone

Comments

@yarikoptic
Copy link

Debian supports (way too ;)) many non x32 architectures, but builds there fail:
https://buildd.debian.org/status/package.php?p=mrtrix3&suite=sid
and quite often simply due to demand of the following header file for SSE support (available only for i386 and amd64)

In file included from cmd/labelconvert.cpp:16:
./core/command.h:20:10: fatal error: xmmintrin.h: No such file or directory
 #include <xmmintrin.h>
          ^~~~~~~~~~~~~
compilation terminated.
(  1/505) [CC] tmp/cmd/labelco

before we go ahead and just blindly restrict architectures, I wondered to ask -- do you see it feasible to support non-SSE ones? I could outline potential benefits if you are interested ;)

jdtournier added a commit that referenced this issue Dec 13, 2018
This otherwise prevents compilation on non x86 platforms (see #1514)
@jdtournier
Copy link
Member

No problem, this one is not actually used for production code... Have a look at #1515, see if that fixes it for you.

@jdtournier
Copy link
Member

I could outline potential benefits if you are interested ;)

No need, but you've caught my interest now... What are we talking about?

@jdtournier
Copy link
Member

One more thing: I notice that many of these builds are failing due to failed dependencies. I assume this is related to Qt? If so, you may still be able to offer a command-line only build: just run ./configure -nogui, and it'll skip the Qt tests and skip building mrview and shview.

@jdtournier
Copy link
Member

And one more while I'm at it: it looks like the install runs from a download of the source code, rather than a git clone. The issue there is that the version information that gets baked into the executables is derived from git describe. So these executables will show up as 'version unknown' in their help page and --version output, which is not ideal. Is there any way to use a git clone here...?

@yarikoptic
Copy link
Author

Is there any way to use a git clone here...?

not directly. But since I will be preparing the source tarball out of the git repo, we could generate a file with versioning information which could be picked up during build. Many projects have such a practice:

$> grep export-subst */.gitattributes 
cctools/.gitattributes:configure export-subst
dipy/.gitattributes:dipy/COMMIT_INFO.txt export-subst
nipy/.gitattributes:nipy/COMMIT_INFO.txt export-subst
nipype/.gitattributes:nipype/COMMIT_INFO.txt export-subst
nipype-master/.gitattributes:nipype/COMMIT_INFO.txt export-subst
pandas/.gitattributes:pandas/_version.py export-subst
pymvpa/.gitattributes:mvpa2/COMMIT_HASH export-subst
pynifti/.gitattributes:nibabel/COMMIT_INFO.txt export-subst
pynifti-localcopy/.gitattributes:nibabel/COMMIT_INFO.txt export-subst

so, as inspired by good old CVS, we get e.g. for pandas:

$> grep  '$Format:' pandas/pandas/_version.py 
    git_refnames = "$Format:%d$"
    git_full = "$Format:%H$"

where it is placing refnames and hexsha inside Python variable right in the code. It would though cause a bit of difficulty in packaging (source tarball might have different expanded lines than I would have in the source tree which would be the git tree) but IIRC avoidable.
Less ambitious, is to have a separate file with that information to be picked up (like in our good old PyMVPA):

$> cat pymvpa/mvpa2/COMMIT_HASH                
$Format:%H$

@yarikoptic
Copy link
Author

I could outline potential benefits if you are interested ;)

No need, but you've caught my interest now... What are we talking about?

oy... ok - two main ones

  1. with all the mobile craze we get all the ARMs around, and they become used even for building low power compute clusters (google is full of hits). So supporting "exotic" architectures now opens the doors to the future
  2. some exotic platforms actually come handy to assure correct operation. E.g. all the big endians, where bytes order is swapped. We had cases where the bug (e.g. as deep as numpy or scipy) was not detected on x32 since only a few conditions were unittested, and incorrect memory access was not detected until bytes got swapped and some benign 0, 1 sequence became 128, 0 or smth like that since one of the bytes was not aligned correctly. So, although assuring correct operation on those platforms sounds like a pain -- it does come handy at times even for the basic QA.

and then still that coolness factor like
image
(and that was way way back) to demonstrate that your software runs virtually everywhere (if someone is concerned about portability)

@yarikoptic
Copy link
Author

No problem, this one is not actually used for production code... Have a look at #1515, see if that fixes it for you.

THANKS! Re "production" -- well, we already uploaded a "non production" version to debian unstable ;)

$> git describe master
3.0_RC3-86-g4b523b413

and current master seems to be as non-production as the previous one:

$> git describe origin/master
3.0_RC3-135-g2b8e7d0c2

so I think we will be fine ;) I will update packaging, and upload, so we will see!

THANKS for quick replies!

@maxpietsch
Copy link
Member

generate a file with versioning information which could be picked up during build

I've had a similar problem with the homebrew package where I had to manually clone and checkout our code into the homebrew repo (instead of simply setting a link to release code tarball) just to get the version string into the commands. How about we check for a file build.version or similar and use the content of that file as the version string in the absence of a working git repository?

@Lestropie
Copy link
Member

How about we check for a file build.version or similar and use the content of that file as the version string in the absence of a working git repository?

We do something like that in BIDS Apps.

Better would be if a text file in the repo contained up-to-date version information, but that file was actually script-generated and CI verified correspondence. However I'm not sure that's possible given the generation of both commit hashes and tags occur after the commit.

E.g. all the big endians, where bytes order is swapped.

We should be covered in that respect. Don't know to what extent it's been tested in recent times (I'm a bit concerned about envvar name mismatch; @jdtournier?), but we've been diligent to cover it off where relevant.

jdtournier added a commit that referenced this issue Dec 14, 2018
This is to allow version information to be updated easily for packaging
purposes (see #1514).
@jdtournier
Copy link
Member

jdtournier commented Dec 14, 2018

Re "production" -- well, we already uploaded a "non production" version to debian unstable ;)

Yes, that's about as 'production' as we've managed so far. Although we do have actual tag releases (still as release candidates though). Trust me, we get a lot less 'production' than that...

I could outline potential benefits if you are interested ;)

No need, but you've caught my interest now... What are we talking about?

oy... ok - two main ones

Yes, I've heard about ARM chips being used for clusters - that's an interesting one to follow. The other reasons are a bit less appealing to me, but interesting nonetheless.

E.g. all the big endians, where bytes order is swapped.

We should be covered in that respect. Don't know to what extent it's been tested in recent times (I'm a bit concerned about envvar name mismatch; @jdtournier?), but we've been diligent to cover it off where relevant.

Well, we're covered in that we've thought about it, but we're not covered in that none of the testing operates on big-endian systems... While early versions of MRtrix were used on big-endian systems, it's been a (long) while. So there's every chance that bugs might be there and have gone undetected for quite some time.

On that note, I think you might be onto something with the envvar name mismatch... Guess that should be MRTRIX_BYTE_ORDER_IS_BIG_ENDIAN, hey? Guess we'll find out pretty quickly if we get one of the big-endian builds to work...


Onto the version string issue:

How about we check for a file build.version or similar and use the content of that file as the version string in the absence of a working git repository?

The problem is if you want that file to store the commit SHA1, then you can't do that without knowing the commit SHA1 in the first place, since the SHA1 depends on the exact contents of the commit. So yes, we can store a version, but then it can't refer to the actual commit SHA1.

Which also means:

Better would be if a text file in the repo contained up-to-date version information, but that file was actually script-generated and CI verified correspondence. However I'm not sure that's possible given the generation of both commit hashes and tags occur after the commit.

Can't be done - same reason, as you guessed here. On top of that, the CI checks out a merge commit as would be pushed to the repo when the pull request is eventually merged, so it's not even the SHA1 of the commit you've pushed, but a different one again... This is why I ended up with the current strategy.

Which leaves @yarikoptic's suggestion:

since I will be preparing the source tarball out of the git repo, we could generate a file with versioning information which could be picked up during build.

That actually sounds like the best plan. This is the bit that updates the version file (core/version.cpp). What we could do is make sure that part can run even without a valid config (currently, it would abort before that stage, but it's an easy fix - see #1516), so all you'd need to do is clone the repo, run ./build (which would abort pretty quickly, but at least update the version file), delete the .git/ folder, and package it up. I've just checked, if there is no git repo to grab the version string from, the ./build script will leave the current one in place, so that part should already work out of the box.

Does that sound do-able...?

@yarikoptic
Copy link
Author

Which leaves @yarikoptic's suggestion:

since I will be preparing the source tarball out of the git repo, we could generate a file with versioning information which could be picked up during build.

That actually sounds like the best plan. This is the bit that updates the version file (core/version.cpp). What we could do is make sure that part can run even without a valid config (currently, it would abort before that stage, but it's an easy fix - see #1516), so all you'd need to do is clone the repo, run ./build (which would abort pretty quickly, but at least update the version file), delete the .git/ folder, and package it up. I've just checked, if there is no git repo to grab the version string from, the ./build script will leave the current one in place, so that part should already work out of the box.

Well, my recommendation was somewhat of a hybrid/in addition to above. What if you do have "$Format:%H$" etc placeholders in your build script, which if finds no $Format...$ which would mean that sources were exported by git archive locally like I would do or by github if someone downloads a zipball, then your ./build just uses them instead of trying to run git etc. This way, if the only way sources would be distributed is either from git archive, or with ./git - you would be all set with the precious git info.

@jdtournier jdtournier added this to the 3.0_RC4 release milestone Jan 30, 2019
@jdtournier jdtournier self-assigned this Jan 30, 2019
@jdtournier jdtournier changed the title platforms with SSE support only? version info on non-git builds & platforms with SSE support only? Feb 5, 2019
@jdtournier
Copy link
Member

jdtournier commented Feb 5, 2019

Based on discussion, suggestion to handle this:

  • add macro called e.g. MRTRIX_BASE_VERSION to the file src/exec_version.h
  • this is set to the tag name for that release, and used by the build script instead of "unknown" if git is not present or no git history
  • to be updated on tag change, process will be to commit tag change, tag the commit, then push
  • in CI, check that tag name matches using git describe --tags --abbrev=0
  • process for tag update to be documented in comment within src/exec_version.h

Thoughts

jhadida pushed a commit to jhadida/mrtrix3 that referenced this issue Feb 16, 2019
This otherwise prevents compilation on non x86 platforms (see MRtrix3#1514)
@jdtournier
Copy link
Member

closed with #1593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants