bmrb-io / PyNMRSTAR

A Python module for reading, writing, and manipulating NMR-STAR files.
MIT License
28 stars 3 forks source link

PyNMRStar fails to install with pip on centos 7.x #121

Closed varioustoxins closed 8 months ago

varioustoxins commented 8 months ago

pynmrstar is broken on centos 7.x and possibly 8.x and anything that uses gcc as opposed to clang or the ms compilers to compile cnmrstar because the c extension has the following construct

For (int I=0;

at line 149?

This is not legal in less than c99 apparently (pip by default uses an older c and requires extra command line flags to make this work)

Also annoyingly the bug doesn’t fully appear in the pip output unless you install with pip —verbose, some how a failure to compile is not getting to the pip machinery…

Attached is a jpeg of the install process failing for reference

IMG_9387

jonwedell commented 8 months ago

Could you check something for me? Can you first install/upgrade wheel and setuptools: pip install --upgrade wheel setuptools and then try the install again? I'm shipping binaries in addition to source, so ideally you shouldn't need to build cnmrstar at all.

varioustoxins commented 8 months ago

Hi Jon

Ok that fixes it. I was using the python distro inside ccpn assign to install it this time (it includes a install_nef_pipelines script)

I will also have a check what happens with the local centos python installation

Would the answer be to tell the users to install the latest pip and this will bring in the latest setup tools with it and be a bit less hack and better for them overall?

regards Gary

Dr Gary S Thompson NMR Facility Manager Wellcome Trust Biomolecular NMR Facility CCPN CoI & working group member School of Biosciences, Division of Natural Sciences University of Kent, Canterbury, Kent, England, CT2 7NZ

☎:01227 82 7117 ✉️: @.*** orchid: orcid.org/0000-0001-9399-7636

On 4 Mar 2024, at 15:50, Jon Wedell @.***> wrote:

You don't often get email from @.*** Learn why this is importanthttps://aka.ms/LearnAboutSenderIdentification

CAUTION: This email originated from outside of the organisation. Do not click links or open attachments unless you recognise the sender and know the content is safe.

Could you check something for me? Can you first install/upgrade wheel and setuptools: pip install --upgrade wheel setuptools and then try the install again? I'm shipping binaries in addition to source, so ideally you shouldn't need to build cnmrstar at all.

— Reply to this email directly, view it on GitHubhttps://github.com/bmrb-io/PyNMRSTAR/issues/121#issuecomment-1976893740, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA3UD6KUNNLTSKTUSHH47NDYWSJ47AVCNFSM6AAAAABEFNWWYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWHA4TGNZUGA. You are receiving this because you authored the thread.Message ID: @.***>

dmaziuk commented 8 months ago

They may have an older glibc, too, if they have a pre-c-99 gcc, in which case the prebuilt binary could fail to load.

{
int I;
(for I ...
}

should make that last-century-proof (in squigglies if you need to limit the I's scope to the for loop)

varioustoxins commented 8 months ago

Hi

Seems like a sensible patch it doesn’t make the c much uglier but it is more portable...

of course there maybe more c99isms present, I haven’t checked further

regards Gary

Dr Gary S Thompson NMR Facility Manager Wellcome Trust Biomolecular NMR Facility CCPN CoI & working group member School of Biosciences, Division of Natural Sciences University of Kent, Canterbury, Kent, England, CT2 7NZ

☎:01227 82 7117 ✉️: @.*** orchid: orcid.org/0000-0001-9399-7636

On 4 Mar 2024, at 17:02, dmaziuk @.***> wrote:

You don't often get email from @.*** Learn why this is importanthttps://aka.ms/LearnAboutSenderIdentification

CAUTION: This email originated from outside of the organisation. Do not click links or open attachments unless you recognise the sender and know the content is safe.

They may have an older glibc, too, if they have a pre-c-99 gcc, in which case the prebuilt binary could fail to load.

{ int I; (for I ... }

should make that last-century-proof (in squigglies if you need to limit the I's scope to the for loop)

— Reply to this email directly, view it on GitHubhttps://github.com/bmrb-io/PyNMRSTAR/issues/121#issuecomment-1977055641, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA3UD6JHNO6EUMNRF35ELS3YWSSJRAVCNFSM6AAAAABEFNWWYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZXGA2TKNRUGE. You are receiving this because you authored the thread.Message ID: @.***>

varioustoxins commented 8 months ago

Hi Jon

so doing a python -m pip install —upgrade pip

Works as well

But I am certainly not compatible with default python3 on centos and you also need root access to install

regards Gary

Dr Gary S Thompson NMR Facility Manager Wellcome Trust Biomolecular NMR Facility CCPN CoI & working group member School of Biosciences, Division of Natural Sciences University of Kent, Canterbury, Kent, England, CT2 7NZ

☎:01227 82 7117 ✉️: @.*** orchid: orcid.org/0000-0001-9399-7636

On 4 Mar 2024, at 15:58, Gary Thompson @.***> wrote:

Hi Jon

Ok that fixes it. I was using the python distro inside ccpn assign to install it this time (it includes a install_nef_pipelines script)

I will also have a check what happens with the local centos python installation

Would the answer be to tell the users to install the latest pip and this will bring in the latest setup tools with it and be a bit less hack and better for them overall?

regards Gary

Dr Gary S Thompson NMR Facility Manager Wellcome Trust Biomolecular NMR Facility CCPN CoI & working group member School of Biosciences, Division of Natural Sciences University of Kent, Canterbury, Kent, England, CT2 7NZ

☎:01227 82 7117 ✉️: @.*** orchid: orcid.org/0000-0001-9399-7636

On 4 Mar 2024, at 15:50, Jon Wedell @.***> wrote:

You don't often get email from @.*** Learn why this is importanthttps://aka.ms/LearnAboutSenderIdentification

CAUTION: This email originated from outside of the organisation. Do not click links or open attachments unless you recognise the sender and know the content is safe.

Could you check something for me? Can you first install/upgrade wheel and setuptools: pip install --upgrade wheel setuptools and then try the install again? I'm shipping binaries in addition to source, so ideally you shouldn't need to build cnmrstar at all.

— Reply to this email directly, view it on GitHubhttps://github.com/bmrb-io/PyNMRSTAR/issues/121#issuecomment-1976893740, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA3UD6KUNNLTSKTUSHH47NDYWSJ47AVCNFSM6AAAAABEFNWWYCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWHA4TGNZUGA. You are receiving this because you authored the thread.Message ID: @.***>

jonwedell commented 8 months ago

Hey Gary,

The first thing I do in a new virtual environment is to upgrade pip, wheel, and setuptools before installing any other required packages - it fixes a lot of issues and so far hasn't caused any. So in response to your question, yes, I would recommend that if anyone is having problems installing PyNMR-STAR that they take those steps first.

Dimitri - thanks for the backwards-compatible suggestion, but I just don't have the bandwidth to support end-of-life platforms.

If the pip/wheel/setuptools solution doesn't work, PyNMR-STAR prior to 3.2 (I believe) has a fall-back pure python parser - or one could fork cnmrstar, fix any issues, install it first, and then install PyNMR-STAR.