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.

Sign up for GitHub

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

Jump to bottom

[BUG FIX] Architecture: arpackdef.h for C/C++, and, arpackicb.h for F77/F90. #249

Merged
merged 1 commit into from
May 31, 2020
Merged

[BUG FIX] Architecture: arpackdef.h for C/C++, and, arpackicb.h for F77/F90. #249

merged 1 commit into from
May 31, 2020

Conversation

fghoussen
Copy link
Collaborator

@fghoussen fghoussen commented Feb 27, 2020 •
edited
Loading

Just checking if this could be the casper bug from #246 ...

@turboencabulator : do you see something wrong (pattern, misuse related to conditional, ...) in TESTS/Makefile.am ?

@coveralls
Copy link

coveralls commented Feb 27, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 1177

  • 34 of 34 (100.0%) changed or added relevant lines in 2 files are covered.
  • 83 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.4%) to 73.728%
Files with Coverage Reduction New Missed Lines %
PARPACK/SRC/MPI/pdsapps.f 1 88.3%
PARPACK/SRC/MPI/pssapps.f 1 88.3%
PARPACK/SRC/MPI/pdsaup2.f 2 61.71%
PARPACK/SRC/MPI/pssaup2.f 2 61.71%
PARPACK/SRC/MPI/pssaitr.f 8 56.91%
SRC/dsaitr.f 8 81.22%
SRC/cnaitr.f 10 81.82%
SRC/znaitr.f 11 81.25%
SRC/cgetv0.f 13 63.54%
SRC/zgetv0.f 13 63.54%
Totals Coverage Status
Change from base Build 1176: -0.4%
Covered Lines: 16397
Relevant Lines: 22240
💛 - Coveralls

@fghoussen
Copy link
Collaborator Author

Found yet another bug nest... In total connection with #246 and maybe also with #230... Pushed a few stuffs to prepare for later work : this may need time. @sylvestre : would be good to finish that before next release, when do you plan next release ?

@sylvestre
Copy link
Contributor

sylvestre commented Feb 28, 2020 via email

@fghoussen fghoussen changed the title Bug3: casper bug ? [BUG FIX] ILP64 Feb 28, 2020
@fghoussen
Copy link
Collaborator Author

When ready :)

OK, cool... Seems there are at least 2 problems here. Short story : #246 is necessary but triggers problems on ILP64. I guess if I get --enable-icb-exmm to work on ILP64 on CI then #230 may be fixed too.

turboencabulator
turboencabulator reviewed Feb 29, 2020
View reviewed changes
TESTS/Makefile.am Outdated
Comment on lines 37 to 38
bug_1315_single_LDADD = $(top_builddir)/SRC/libarpack$(LIBSUFFIX).la
bug_1315_double_LDADD = $(top_builddir)/SRC/libarpack$(LIBSUFFIX).la
bug_1315_single_LDADD = $(LDADD)
bug_1315_double_LDADD = $(LDADD)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should stay the way they were, see ddfd843. But I'm not familiar with the problem that this tries to solve.

turboencabulator
turboencabulator reviewed Feb 29, 2020
View reviewed changes
TESTS/Makefile.am Outdated
Comment on lines 28 to 32
icb_arpack_c_LDADD = $(top_builddir)/SRC/libarpack$(LIBSUFFIX).la
icb_arpack_c_LDADD = $(LDADD)
icb_arpack_c_CPPFLAGS = $(AM_CPPFLAGS) -I$(top_builddir) -I$(top_srcdir)/ICB

icb_arpack_cpp_SOURCES = icb_arpack_cpp.cpp
icb_arpack_cpp_LDADD = $(top_builddir)/SRC/libarpack$(LIBSUFFIX).la
icb_arpack_cpp_LDADD = $(LDADD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try deleting these two LDADD lines instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, $(LDADD) is the default, so you don't need to specify it on each target like this. I don't know what problem this is trying to solve, so I can't comment on whether changing the value is the right thing to do.

Copy link
Collaborator Author

@fghoussen fghoussen Feb 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@turboencabulator : you are right, I've just reverted it (since trying to test pyarpack on ILP64, lots of problems show up and mix up at the same time... I though this could explain one of them : I understood later that I was wrong). For sake of symmetry, and if I get that well, I tried to push 9ab2463

@fghoussen
Copy link
Collaborator Author

Doing nothing for now, but, preparing for the fix which may not be so easy

@fghoussen
Copy link
Collaborator Author

OK, so if I am not mistaken, I should get stability (accuracy test) and clean traces to debug problems on ILP64 CI : this should now fail on the one problem that still need to be fixed...

@fghoussen
Copy link
Collaborator Author

Short story : arpackdef.h must be split in 2 files, one for C/C++, another for F77/F90.

Long story:

  • on master, arpackdef.h has both #define for F77/F90/C/C++ : this is bad but worked as long as there are only #define in it => all OK on LP64 and ILP64 on master

  • compiling with --enable-icb-exmm on ILP64 needs to #include <file.h> in arpackdef.h so that F77/F90 can no more compile this => [BUG FIX - architecture] fix arpackdef.h: must not be included by F77/F90. #246 => all OK on LP64 but KO on ILP64

  • but now F77/F90 needs his own arpackicb.h (handled by arpackdef.h before) to define i_int for ICB => this what this PR is about => should be back all OK on LP64 and ILP64

Hope this should be enough to get --enable-icb-exmm to work on ILP64.

@fghoussen
Copy link
Collaborator Author

CI not triggered ?... Rebasing on master

@fghoussen fghoussen changed the title [BUG FIX] ILP64 [BUG FIX] Architecture: arpackdef.h for C/C++, and, arpackicb.h for F77/F90. Feb 29, 2020
@fghoussen fghoussen mentioned this pull request Feb 29, 2020
Closed
@fghoussen fghoussen force-pushed the bug3 branch 2 times, most recently from 43f55cd to 4bafd3e Compare March 3, 2020 16:51
@fghoussen
Copy link
Collaborator Author

Rebased on master as it was conflicting.

@fghoussen
Copy link
Collaborator Author

2d rebase on master as it conflicts again. could you merge that ? will conflict again with #258 if icb tests are indeed upgraded...

@fghoussen
Copy link
Collaborator Author

Good to go ! Merge it before next release ! :D

@sylvestre
Copy link
Contributor

Sorry, I missed it

@sylvestre sylvestre merged commit 160b572 into opencollab:master May 31, 2020
@fghoussen fghoussen deleted the bug3 branch May 31, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turboencabulator turboencabulator turboencabulator left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fghoussen @coveralls @sylvestre @turboencabulator