Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

OpenSuse Tumbleweed gcc (v. 5.1.1) -dumpversion issues #25671

Closed
wants to merge 1 commit into from

Conversation

Rynaro
Copy link

@Rynaro Rynaro commented Jul 13, 2015

Hello,
I found some difficulties when attempting to configure and compile NodeJS packages in openSUSE Tumbleweed.
The error reported when running ./configure

Traceback (most recent call last):
  File "./configure", line 992, in <module>
    configure_node(output)
  File "./configure", line 530, in configure_node
    o['variables']['gcc_version'] = 10 * cc_version[0] + cc_version[1]
IndexError: tuple index out of range

In the configure file, specifically the compiler_version function. It returned a tuple to cc_version variable. However, in OpenSUSE, if we execute:

henrique@aegis~>: gcc -dumpversion
5

It returns a single integer value, making the variable that contains the tuple store (5,), resulting in IndexError exception.

I made this change in my configure file, allowing the compilation of NodeJS in my Tumbleweed environment.

ps: Excuse me for my English, it is not my native language. I'm learning it yet. :)

@Rynaro Rynaro changed the title OpenSuse Tumbleweed gcc -bindversion issues OpenSuse Tumbleweed gcc -dumpversion issues Jul 13, 2015
@Rynaro Rynaro changed the title OpenSuse Tumbleweed gcc -dumpversion issues OpenSuse Tumbleweed gcc (v. 5.1.1) -dumpversion issues Jul 13, 2015
@fusion809
Copy link

I received the same error you reported here and with your modification to the configure file I was able to install node, thanks.

@jasnell
Copy link
Member

jasnell commented Aug 6, 2015

This looks fine to me, but let's see what @orangemocha or @misterdjules think

@jasnell jasnell added the build label Aug 6, 2015
@orangemocha
Copy link
Contributor

Interesting. So there is no other way of telling the minor version (.1) on this platform?

@orangemocha
Copy link
Contributor

The change per se LGTM. I probably would have implemented it with an if statement but the try-except works.

@Rynaro
Copy link
Author

Rynaro commented Aug 10, 2015

@orangemocha. By default gcc 5.1+ dont returns to me a minor version of their compiler. I dont know if they have plans to reinsert the minor version in forward releases.


try:
o['variables']['gcc_version'] = 10 * cc_version[0] + cc_version[1]
except IndexError:

Choose a reason for hiding this comment

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

If we are expecting at least one element then we can do if len(cc_version) == 2: ... else: ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Or:

o['variables']['gcc_version'] = 10 * cc_version[0]
if len(cc_version) >= 2:
  o['variables']['gcc_version'] += cc_version[1]
else:
  print 'Warning: no gcc minor version found, assuming 0'

Choose a reason for hiding this comment

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

Ya something like this. I didn't have a computer at that moment, so couldn't express myself clearly. Sorry.

Copy link
Author

Choose a reason for hiding this comment

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

o['variables']['gcc_version'] = 10 * cc_version[0]
This line always run, because cc_version[0] refers the major version of gcc.
Now I think, we only need to check if cc_version[1] exists. If exists, use your value, if not exists show a warning message. Like the code above.
I've used try .. except because Python encourages use of try.
I've tested if .. else way and my try ... except way, both ways have solved this issue.

Choose a reason for hiding this comment

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

I agree that Python encourages EAFP: Easier to ask for forgiveness than permission. But I wanted to make sure that the code is easily understandable. Anyway I am not going to hold this PR with this discussion. Please continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Will land as is.

@Rynaro
Copy link
Author

Rynaro commented Aug 11, 2015

@thefourtheye . Really, the if ... else statement, is a great solution for this issue. But, try ... except is better performatic than if ... else. And Python encourages the use of exceptions in most of cases.
The if ... else works too, but I used try ... except in this commit, and I have no problem to change this code try .. except to if .. else.
😄

@thefourtheye
Copy link

I was worried that if the number of elements is zero then we ll not know which access throws the error.

@orangemocha
Copy link
Contributor

Just noticed that this PR is against master. If it is meant to go in v0.12, then it should be against the v0.12 branch. Otherwise, it probably makes more sense to do it in io.js master. @jasnell @misterdjules is that right?

@jasnell
Copy link
Member

jasnell commented Aug 11, 2015

Yes. Commits here need to be targeted at v0.12 or v0.10. Commits against master need to go into nodejs/io.js moving forward.

@orangemocha
Copy link
Contributor

@jasnell : makes sense, thank you. Assuming that 'being able to compile' is one of those fixes that we want to include in v0.10, this should land in v0.10 first. I can do the cherry-picking.

orangemocha pushed a commit that referenced this pull request Aug 11, 2015
PR-URL: #25671
Reviewed-By: Alexis Campailla <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@orangemocha
Copy link
Contributor

Landed in 8564a9f.

michaelni pushed a commit to FFmpeg/FFmpeg that referenced this pull request Aug 24, 2015
grep is not required for the functionality in this instance.
This avoids an unnecessary fork, and also avoids a duplicated dumpversion call.
Furthermore, it also corrects behavior when no minor version number is present, see e.g
nodejs/node-v0.x-archive#25671.

Signed-off-by: Ganesh Ajjanagadde <[email protected]>
Signed-off-by: Michael Niedermayer <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants