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

BLD: Enable build on AIX #8173

Merged
merged 2 commits into from
Oct 22, 2016
Merged

BLD: Enable build on AIX #8173

merged 2 commits into from
Oct 22, 2016

Conversation

aixtools
Copy link
Contributor

modifying two setup.py files to define _LARGE_FILES if sys.platform[:3] == aix: resolves this issue.

if sys.platform[:3] == "aix":
defs = [('_LARGE_FILES', None)]
else:
defs = [('_FILE_OFFSET_BITS', '64'),
('_LARGEFILE_SOURCE', '1'),
Copy link
Member

Choose a reason for hiding this comment

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

Indentation. the list contents should align vertically.

@charris
Copy link
Member

charris commented Oct 19, 2016

Minor style nit. Is this likely to be AIX/Python version dependent? Could you also squash the commits and follow the commit message guidelines in doc/source/dev/gitwash/development_workflow.rst? In particular, make sure the body of the commit message explains the problem and for what platform. I'm curious as to why this is needed for numpy.random. Is there a way to be sure that it isn't needed elsewhere?

@charris charris changed the title Issue8118 ENH: Enable build on AIX, issue #8118 Oct 19, 2016
@charris charris changed the title ENH: Enable build on AIX, issue #8118 ENH: Enable build on AIX Oct 19, 2016
@aixtools
Copy link
Contributor Author

I would repeat the test, but my NAS died - so I am 2000km away, and waiting for someone to come home and reboot the NAS - hopefully the degraded array will be awake long enough to rebuild the array.

As to needed in random - was a surprise to me as well. I thought I was finished when I had done "core". But a test build failed in the new area and I looked in all the setup.py files for "LARGE" settings. Only random/setup.py had the same defines as core/setup.py just different way of specifying them.

As to comments, deleting them or modifying them - I only started using git yesterday. I was happy that I even got it pushed to my fork.

Thank you for the wise and friendly words!

FYI: the setting, better define, is compiler independent - same issue occurs with both gcc and xlc.

@charris
Copy link
Member

charris commented Oct 19, 2016

@aixtools The process for squashing and rewriting commit messages goes as follows for your three commits. First git rebase -i HEAD^^^, that will bring up the editor and you should replace the first pick with r and the rest with f, save and exit. The 2nd and 3rd commits will be squashed into the first and then the editor will come up again so you can edit the commit message. Second, do a force push to origin git push -f origin HEAD.

@aixtools
Copy link
Contributor Author

thx @charris - will work on that. Server (NAS disk died) yesterday, so in process of rebuilding.
FYI: the error message encountered in the "random" module is same (just different file) as for "core".

Last (key) message:
"/usr/include/unistd.h", line 921.25: 1506-343 (S) Redeclaration of fclear64 differs from previous declaration on line 918 of "/usr/include/unistd.h".
"/usr/include/unistd.h", line 921.25: 1506-050 (I) Return type "long long" in redeclaration is not compatible with the previous return type "long".
"/usr/include/unistd.h", line 921.25: 1506-377 (I) The type "long long" of parameter 2 differs from the previous type "long".
"/usr/include/unistd.h", line 922.25: 1506-343 (S) Redeclaration of fsync_range64 differs from previous declaration on line 919 of "/usr/include/unistd.h".
"/usr/include/unistd.h", line 922.25: 1506-377 (I) The type "long long" of parameter 3 differs from the previous type "long".
error: Command "xlc_r -I/opt/include -O3 -qmaxmem=-1 -qarch=pwr5 -q64 -I/opt/buildaix/includes -DNDEBUG -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE=1 -D_LARGEFILE64_SOURCE=1 -Inumpy/core/include -Ibuild/src.aix-5.3-2.7/numpy/core/include/numpy -Inumpy/core/src/private -Inumpy/core/src -Inumpy/core -Inumpy/core/src/npymath -Inumpy/core/src/multiarray -Inumpy/core/src/umath -Inumpy/core/src/npysort -I/opt/include/python2.7 -Ibuild/src.aix-5.3-2.7/numpy/core/src/private -Ibuild/src.aix-5.3-2.7/numpy/core/src/private -Ibuild/src.aix-5.3-2.7/numpy/core/src/private -c numpy/random/mtrand/distributions.c -o build/temp.aix-5.3-2.7/numpy/random/mtrand/distributions.o" failed with exit status 1

Further, my commit message will be something in the line of:
"""BLD: AIX uses the flag _LARGE_FILES to ensure proper prototype declarations for 32-bit and 64-bit functions (e.g., fclear() and fclear64()) to access/manipulate "large files".

As early as 1997 AIX started supporting so-called "large files", i.e., length > signed 32-bit. The convention became define flag _LARGE_FILES - before including any system include files (so that no relevant function calls would be redefined later in the build process).
At present, the numpy code only applies the flags known in "non-AIX" environments (i.e., I do not know if HPUX, Solaris, etc. use one or more of the current flags, or have yet other flags).
This change should have no impact on version of python used - it accomplishes the same things as
defining _FILE_OFFSET_BITS, _LARGEFILE_SOURCE, and _LARGEFILE64_SOURCE does for other platforms.

The "cleanup" is yet to come. If the commit message is missing something you feel is missing, please advise.

@charris charris changed the title ENH: Enable build on AIX BLD: Enable build on AIX Oct 20, 2016
… declarations

The problem this fix resolves is to ensure that 32-bit and 64-bit functions
(e.g., fclear() and fclear64()) to access/manipulate "large files" are
defined properly - much as GNU and other platforms use one or more of
the defines _FILE_OFFSET_BITS, _LARGEFILE_SOURCE, and _LARGEFILE64_SOURCE.

Without this fix the numpy code only defines flags that are not recognized
in the AIX environment and have no effect in anyway.
The fix applies the AIX convention and does not "export" any of the flags
currently exported. For all other platforms the current flags:
_FILE_OFFSET_BITS, _LARGEFILE_SOURCE, and _LARGEFILE64_SOURCE are "exported".

This fix should not have any impact or side-effect based on the version
of python used.

History:
Starting around 1997 AIX started supporting so-called "large files",
i.e., length > signed 32-bit. In the period 1997-1998 the flag _LARGE_FILES
was established to simplify porting 32-bit applications to 64-bit.
The convention is to define _LARGE_FILES before including any
"system include files" either as an argument to ${CC} (e.g., in ${CFLAGS}
or as the first #define in every source file. This is to ensure that
that no relevant function calls would be redefined later in the build process.
@aixtools
Copy link
Contributor Author

I have done the "squash" and message update as requested. I did not create a new pull request. Is that (also) needed?

re: your final question: "Is there a way to be sure that it isn't needed elsewhere?" - basically, anywhere you need (one of) _FILE_OFFSET_BITS, _LARGEFILE_SOURCE, and _LARGEFILE64_SOURCE - _LARGE_FILES should be used for AIX. These current defines do not exist in AIX system files.

@charris
Copy link
Member

charris commented Oct 22, 2016

I fixed up the indentation. That will modify your branch on origin. Yes, Github gives committers access to the PR branch ;) Just thought I'd give it a try.

@charris charris merged commit a5db940 into numpy:master Oct 22, 2016
@charris
Copy link
Member

charris commented Oct 22, 2016

Thanks @aixtools .

@rgommers rgommers added this to the 1.12.0 release milestone Oct 22, 2016
@rgommers
Copy link
Member

@aixtools does this mean we can close the numpy and scipy issues you opened, or is there more to do?

@aixtools
Copy link
Contributor Author

The second issue with scipy (also _LARGE_FILES related imho) will first need similar treatment. But the first part, i.e., the first message scipy #6685 is not handled yet (as I replied by email, numpy has a similiar name issue with added _ (underscrore) to lapack names.

@aixtools aixtools deleted the issue8118 branch October 23, 2016 13:57
@aixtools
Copy link
Contributor Author

excuse me for asking: I am not smart enough to find, or worse understand the documentation re: how I get/keep my forked copy in sync.
My copy says: This branch is 38 commits behind numpy:master.
I do not find, or understand the "workflow" document on how to get this back in sync (delete and fork again feels wrong).

@charris
Copy link
Member

charris commented Oct 23, 2016

Usually you have entries in you local repository for "origin", which is your githup forked numpy repo, and "upstream", which is the numpy github repo. I keep my local repo up to date by pulling from upstream, i.e., if I have master checked out git pull upstream master and then update my fork with git push origin master. This can be shortened a bit depending on the entries in your local .git/config file. For instance, mine begins

[core]
    repositoryformatversion = 0
    filemode = true
    bare = false
    logallrefupdates = true
[remote "origin"]
    url = git@github.com:charris/numpy.git
    fetch = +refs/heads/*:refs/remotes/origin/*
[remote "upstream"]
    url = git@github.com:numpy/numpy
    fetch = +refs/heads/*:refs/remotes/upstream/*

The git@ url form is for ssh git protocol, you won't have that for master and will need the https:// form I think. I fiddle with my config, so that might not be standard with everyone.

@rgommers
Copy link
Member

The development docs could be clearer on how to keep master in sync, I added something in gh-8205.

@aixtools
Copy link
Contributor Author

On 23-Oct-16 16:23, Charles Harris wrote:

Usually

Usually noobs such as myself have no clue...

The info below is enlightening. I cannot place it all immediately, but
it does provide "hooks" for my further study. IMHO it could be useful
for others like myself who are new git (as well as numpy/scipy) and
adding "this" to the workflow document could be useful.
Note: the only part of workflow document I have read is the "how to
comment" part you sent a link to. As I already had a fork and could "git
clone" my fork, and push to that, and PR from that - I thought I had
done everything I needed to to work with git and projects.

But my lack of understanding git - ;) might not be standard for everyone!

Thanks for the homework (tomorrow).

you have entries in you local repository for "origin", which is your
githup forked numpy repo, and "upstream", which is the numpy github
repo. I keep my local repo up to date by pulling from upstream, i.e.,
if I have master checked out |git pull upstream master| and then
update my fork with |git push origin master|. This can be shortened a
bit depending on the entries in your local |.git/config| directory.
For instance, mine begins

|[core] repositoryformatversion = 0 filemode = true bare = false
logallrefupdates = true [remote "origin"] url =
git@github.com:charris/numpy.git fetch =
+refs/heads/:refs/remotes/origin/ [remote "upstream"] url =
git@github.com:numpy/numpy fetch = +refs/heads/:refs/remotes/upstream/ |

The |git@| url form is for ssh git protocol, you won't have that for
master and will need the |https://| form I think. I fiddle with my
config, so that might not be standard with everyone.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8173 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AKWY9Url-pIrVoO-BU_2A2ub4HwX0dznks5q23v5gaJpZM4KaTGS.

@aixtools
Copy link
Contributor Author

On 22-Oct-16 23:02, Ralf Gommers wrote:

@aixtools https://github.com/aixtools does this mean we can close
the numpy and scipy issues you opened, or is there more to do?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8173 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AKWY9fRyyYzPLBmw8K4D8mHiLu7hqiwAks5q2nnUgaJpZM4KaTGS.

This was specific to one issue with AIX (_LARGE_FILES). - So, for that
on numpy - yes close. For scipy I would have to review again.

From memory there is a "common" issue I am running into when I use the
lapack library. numpy also has issues with an additional _ in the name
the loader is looking for. I only ran into it on numpy because I had
left the lapack libraries on the system as I debugged this issue.

So: a) I shall review the scipy issues I have opened and close it if I
believe it is appropriate to close

b) for lapack and numpy I shall open a new issue when I have time to
work on it. I hope this week.

Michael

@rgommers
Copy link
Member

Thanks Michael. Will close gh-8118 then.

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

Successfully merging this pull request may close these issues.

None yet

3 participants