Skip to content

v1.10: fortran and misc fixes #2281

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
Merged
merged 9 commits into from
May 8, 2017
Merged

v1.10: fortran and misc fixes #2281

merged 9 commits into from
May 8, 2017

Conversation

@ggouaillardet
Copy link
Contributor

@ggouaillardet ggouaillardet commented Oct 24, 2016

No description provided.

@ggouaillardet ggouaillardet added this to the v1.10.5 milestone Oct 24, 2016
jsquyres
jsquyres previously requested changes Oct 27, 2016
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

@ggouaillardet Sorry to pester, but this PR needs signed-off-by lines, too...

@ggouaillardet
fortran/use-mpi-ignore-trk: fix typo in MPI_File_read_ordered_begin i…
8bcb3a2 
…nterface

(cherry picked from commit open-mpi/ompi@e02dc1e)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>
@ggouaillardet
fortran/use-mpi-ignore-tkr: fix typo in MPI_File_write_at_all_begin i…
1ba7519 
…nterface

(cherry picked from commit open-mpi/ompi@e2769e4)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>
@ggouaillardet
fortran: correctly defines MPI_DISPLACEMENT_CURRENT with KIND=MPI_OFF…
2b504a2 
…SET_KIND

and remove unused ompi/include/mpif-mpi-io.h

(back-ported from commit open-mpi/ompi@055df6f)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>
@ggouaillardet
man: fix typos in MPI_Info_get_{nkeys,nthkey}
1e0cf38 
Thanks Nicolas Joly for the patch

(cherry picked from commit open-mpi/ompi@98f6269)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>
@ggouaillardet
fortran/use-mpi-tkr: update .gitignore
2400dfd 
ignore automatically generated mpi-tkr-sizeof.*

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(back-ported from commit open-mpi/ompi@52a1f96)
@ggouaillardet
fortran/use-mpi-tkr: rename mpi-f90-cptr-interfaces.F90 into mpi-f90-…
9befd81 
…cptr-interfaces.h

this file is meant to be included and not compiled, so use a consistent naming

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(back-ported from commit open-mpi/ompi@5543b19)
@ggouaillardet
fortran/use-mpi-tkr: only build MPI_File support if requested
d6c07b8 
Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(cherry picked from commit open-mpi/ompi@8e26e78)
@ggouaillardet
fortran/use-mpi-trk: add PMPI_* interfaces in mpi module
19763a0 
Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(cherry picked from commit open-mpi/ompi@1a16e68)
@ggouaillardet
fortran/use-mpi-tkr: fix MPI_Sizeof handling
18c8d67 
MPI_Sizeof related stuff has been moved to their own files.
Remove MPI_Sizeof from Fortran interfaces when it cannot be built
(e.g. stock gcc 4.8 on CentOS 7)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

(cherry picked from commit open-mpi/ompi@bf78976)
@ggouaillardet ggouaillardet force-pushed the topic/v1.10/fortran_and_misc_fixes branch from 0c507d2 to 18c8d67 Compare October 28, 2016 01:57
@ggouaillardet ggouaillardet dismissed jsquyres’s stale review October 28, 2016 07:27

added Signed-off-by

@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Oct 28, 2016

@jsquyres i also added the use-mpi-tkr fixes

@jsquyres
Copy link
Member

jsquyres commented Oct 28, 2016

bot:lanl:retest

@rhc54
Copy link
Contributor

rhc54 commented Oct 31, 2016

@jsquyres review??

@jsquyres jsquyres modified the milestones: v1.10.5, v1.10.6 Jan 3, 2017
@rhc54
Copy link
Contributor

rhc54 commented Jan 26, 2017

@jsquyres Need for 1.10.6?

@rhc54 rhc54 modified the milestones: v1.10.7, v1.10.6 Feb 21, 2017
@rhc54
Copy link
Contributor

rhc54 commented May 6, 2017

@jsquyres I think at this point it is safe to say nobody cares about this for the 1.10 series? If they do, then can we get this reviewed?

jsquyres
jsquyres approved these changes May 8, 2017
View reviewed changes
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

All the commits that are on this PR are good. However, it also needs d32eff6 (adding MPI_AINT_* to mpif-externals.h). For expediency, I will do that in a separate PR.

@jsquyres jsquyres mentioned this pull request May 8, 2017
Merged
@rhc54 rhc54 merged commit 01b97ab into open-mpi:v1.10 May 8, 2017
@jsquyres
Copy link
Member

jsquyres commented May 15, 2017

Oh shoot, 2b504a2 from this PR will change the ABI (because we change the type of MPI_DISPLACEMENT_CURRENT.

@rhc54 I'm not quite sure what to do here. On the one hand, we had the type of the variable wrong. And Fortran is a strongly typed language, so correct MPI programs should not have been compiling previously. ...but they may have adapted their programs to use our (incorrect) type. But from the "this was a bug" perspective, I think we're ok to release.

...but then the question becomes: what should the .so version number be? Do we actually bump the version such that the new .so version number marks that the prior version is incompatible? I'm inclined to say "no" -- should we indicate that the library has changed, but it is still compatible, even though, technically speaking, that's a lie. I think that this one variable is fairly obscure and so few people use the Fortran modules that we're ok.

We should probably add something in NEWS and/or README about this, though.

Thoughts?

@jsquyres
Copy link
Member

jsquyres commented May 15, 2017 •
edited
Loading

Also, technically, 1ba7519 renames two APIs (which were clearly wrong, but still -- it changes the ABI because we didn't leave the old names accessible).

@rhc54
Copy link
Contributor

rhc54 commented May 15, 2017

Given that these are fairly obscure, and that the changes were actually bug fix/corrections to match the standard, I'm comfortable with just indicating that the library changed and adding something in the NEWS about it

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

Reviewers

@jsquyres jsquyres jsquyres approved these changes

Assignees

@jsquyres jsquyres

Labels

bug RM approved

Projects

None yet

Milestone

v1.10.7

Development

Successfully merging this pull request may close these issues.

3 participants

@ggouaillardet @jsquyres @rhc54