Discussion:
[gentoo-dev] [PATCH] fortran-2.eclass: support EAPI 7
Andrew Savchenko
2018-10-27 22:38:41 UTC
Permalink
Hi all!

The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.

Looks like no other EAPI 7 specific changes needed.

diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..1baf776e368a 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2017 Gentoo Foundation
+# Copyright 1999-2018 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

# @ECLASS: fortran-2.eclass
@@ -8,7 +8,7 @@
# @AUTHOR:
# Author Justin Lecher <***@gentoo.org>
# Test functions provided by Sebastien Fabbro and Kacper Kowalik
-# @SUPPORTED_EAPIS: 4 5 6
+# @SUPPORTED_EAPIS: 4 5 6 7
# @BLURB: Simplify fortran compiler management
# @DESCRIPTION:
# If you need a fortran compiler, then you should be inheriting
this eclass. @@ -27,10 +27,10 @@
#
# FORTRAN_NEED_OPENMP=1

-inherit eutils toolchain-funcs
+inherit toolchain-funcs

case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac


Best regards,
Andrew Savchenko
Michał Górny
2018-10-28 18:29:28 UTC
Permalink
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
--
Best regards,
Michał Górny
Andrew Savchenko
2018-10-29 00:57:05 UTC
Permalink
Post by Michał Górny
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..1baf776e368a 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -1,285 +1,285 @@
-# Copyright 1999-2017 Gentoo Foundation
+# Copyright 1999-2018 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

# @ECLASS: fortran-2.eclass
# @MAINTAINER:
# ***@gentoo.org
# ***@gentoo.org
# @AUTHOR:
# Author Justin Lecher <***@gentoo.org>
# Test functions provided by Sebastien Fabbro and Kacper Kowalik
-# @SUPPORTED_EAPIS: 4 5 6
+# @SUPPORTED_EAPIS: 4 5 6 7
# @BLURB: Simplify fortran compiler management
# @DESCRIPTION:
# If you need a fortran compiler, then you should be inheriting this eclass.
# In case you only need optional support, please export FORTRAN_NEEDED before
# inheriting the eclass.
#
# The eclass tests for working fortran compilers
# and exports the variables FC and F77.
# Optionally, it checks for extended capabilities based on
# the variable options selected in the ebuild
# The only phase function exported is fortran-2_pkg_setup.
# @EXAMPLE:
# FORTRAN_NEEDED="lapack fortran"
#
# inherit fortran-2
#
# FORTRAN_NEED_OPENMP=1

-inherit eutils toolchain-funcs
+inherit toolchain-funcs

case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac

if [[ ! ${_FORTRAN_2_CLASS} ]]; then

# @ECLASS-VARIABLE: FORTRAN_NEED_OPENMP
# @DESCRIPTION:
# Set to "1" in order to automatically have the eclass abort if the fortran
# compiler lacks openmp support.
: ${FORTRAN_NEED_OPENMP:=0}

# @ECLASS-VARIABLE: FORTRAN_STANDARD
# @DESCRIPTION:
# Set this, if a special dialect needs to be supported.
# Generally not needed as default is sufficient.
#
# Valid settings are any combination of: 77 90 95 2003
: ${FORTRAN_STANDARD:=77}

# @ECLASS-VARIABLE: FORTRAN_NEEDED
# @DESCRIPTION:
# If your package has an optional fortran support, set this variable
# to the space separated list of USE triggering the fortran
# dependency.
#
# e.g. FORTRAN_NEEDED=lapack would result in
#
# DEPEND="lapack? ( virtual/fortran )"
#
# If unset, we always depend on virtual/fortran.
: ${FORTRAN_NEEDED:=always}

for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
DEPEND+=" virtual/fortran"
RDEPEND+=" virtual/fortran"
break
;;
no)
break
;;
test)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
*)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
RDEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
esac
done
unset _f_use

# @FUNCTION: fortran_int64_abi_fflags
# @DESCRIPTION:
# Return the Fortran compiler flag to enable 64 bit integers for
# array indices
# @CODE
fortran_int64_abi_fflags() {
debug-print-function ${FUNCNAME} "${@}"

_FC=$(tc-getFC)
if [[ ${_FC} == *gfortran* ]]; then
echo "-fdefault-integer-8"
elif [[ ${_FC} == ifort ]]; then
echo "-integer-size 64"
else
die "Compiler flag for 64bit interger for ${_FC} unknown"
fi
}

# @FUNCTION: _fortran_write_testsuite
# @INTERNAL
# @DESCRIPTION:
# writes fortran test code
_fortran_write_testsuite() {
debug-print-function ${FUNCNAME} "${@}"

local filebase=${T}/test-fortran

# f77 code
cat <<- EOF > "${filebase}.f"
end
EOF

# f90/95 code
cat <<- EOF > "${filebase}.f90"
end
EOF

# f2003 code
cat <<- EOF > "${filebase}.f03"
procedure(), pointer :: p
end
EOF
}

# @FUNCTION: _fortran_compile_test
# @USAGE: <compiler> [dialect]
# @INTERNAL
# @DESCRIPTION:
# Takes fortran compiler as first argument and dialect as second.
# Checks whether the passed fortran compiler speaks the fortran dialect
_fortran_compile_test() {
debug-print-function ${FUNCNAME} "${@}"

local filebase=${T}/test-fortran
local fcomp=${1}
local fdia=${2}
local fcode=${filebase}.f${fdia}
local ret

[[ $# -lt 1 ]] && \
die "_fortran_compile_test() needs at least one argument"

[[ -f ${fcode} ]] || _fortran_write_testsuite

${fcomp} "${fcode}" -o "${fcode}.x" \
Post by Michał Górny
Post by Andrew Savchenko
"${T}"/_fortran_compile_test.log 2>&1
ret=$?

rm -f "${fcode}.x"
return ${ret}
}

# @FUNCTION: _fortran-has-openmp
# @RETURN: return code of the compiler
# @INTERNAL
# @DESCRIPTION:
# See if the fortran supports OpenMP.
_fortran-has-openmp() {
debug-print-function ${FUNCNAME} "${@}"

local flag
local filebase=${T}/test-fc-openmp
local fcode=${filebase}.f
local ret
local _fc=$(tc-getFC)

cat <<- EOF > "${fcode}"
call omp_get_num_threads
end
EOF

for flag in -fopenmp -xopenmp -openmp -mp -omp -qsmp=omp; do
${_fc} ${flag} "${fcode}" -o "${fcode}.x" \
&>> "${T}"/_fortran_compile_test.log
ret=$?
(( ${ret} )) || break
done

rm -f "${fcode}.x"
return ${ret}
}

# @FUNCTION: _fortran_die_msg
# @INTERNAL
# @DESCRIPTION:
# Detailed description how to handle fortran support
_fortran_die_msg() {
debug-print-function ${FUNCNAME} "${@}"

echo
eerror "Please install currently selected gcc version with USE=fortran."
eerror "If you intend to use a different compiler then gfortran, please"
eerror "set FC variable accordingly and take care that the necessary"
eerror "fortran dialects are supported."
echo
die "Currently no working fortran compiler is available (see ${T}/_fortran_compile_test.log for information)"
}

# @FUNCTION: _fortran_test_function
# @INTERNAL
# @DESCRIPTION:
# Internal test function for working fortran compiler.
# It is called in fortran-2_pkg_setup.
_fortran_test_function() {
debug-print-function ${FUNCNAME} "${@}"

local dialect

: ${F77:=$(tc-getFC)}

: ${FORTRAN_STANDARD:=77}
for dialect in ${FORTRAN_STANDARD}; do
case ${dialect} in
77) _fortran_compile_test $(tc-getF77) || \
_fortran_die_msg ;;
90|95) _fortran_compile_test $(tc-getFC) 90 || \
_fortran_die_msg ;;
2003) _fortran_compile_test $(tc-getFC) 03 || \
_fortran_die_msg ;;
2008) die "Future" ;;
*) die "${dialect} is not a Fortran dialect." ;;
esac
done

tc-export F77 FC
einfo "Using following Fortran compiler:"
einfo " F77: ${F77}"
einfo " FC: ${FC}"

if [[ ${FORTRAN_NEED_OPENMP} == 1 ]]; then
if _fortran-has-openmp; then
einfo "${FC} has OPENMP support"
else
die "Please install current gcc with USE=openmp or set the FC variable to a compiler that supports OpenMP"
fi
fi
}

# @FUNCTION: _fortran-2_pkg_setup
# @INTERNAL
# @DESCRIPTION:
# _The_ fortran-2_pkg_setup() code
_fortran-2_pkg_setup() {
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
_fortran_test_function && break
;;
no)
einfo "Forcing fortran support off"
break
;;
*)
if use ${_f_use}; then
_fortran_test_function && break
else
unset FC
unset F77
fi
;;
esac
done
}


# @FUNCTION: fortran-2_pkg_setup
# @DESCRIPTION:
# Setup functionality,
# checks for a valid fortran compiler and optionally for its openmp support.
fortran-2_pkg_setup() {
debug-print-function ${FUNCNAME} "${@}"

if [[ ${MERGE_TYPE} != binary ]]; then
_fortran-2_pkg_setup
fi
}

_FORTRAN_2_ECLASS=1
fi


Best regards,
Andrew Savchenko
Michał Górny
2018-10-30 07:18:58 UTC
Permalink
Post by Andrew Savchenko
Post by Michał Górny
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
I'm going to include a few 'easy cleanup' comments since EAPI 7
is a good opportunity to improve the eclass. I'm going to skip horribly
bad design decisions since I suppose nobody cares.
Post by Andrew Savchenko
diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..1baf776e368a 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -1,285 +1,285 @@
-# Copyright 1999-2017 Gentoo Foundation
+# Copyright 1999-2018 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2
# Test functions provided by Sebastien Fabbro and Kacper Kowalik
# If you need a fortran compiler, then you should be inheriting this eclass.
# In case you only need optional support, please export FORTRAN_NEEDED before
# inheriting the eclass.
#
# The eclass tests for working fortran compilers
# and exports the variables FC and F77.
# Optionally, it checks for extended capabilities based on
# the variable options selected in the ebuild
# The only phase function exported is fortran-2_pkg_setup.
# FORTRAN_NEEDED="lapack fortran"
#
# inherit fortran-2
#
# FORTRAN_NEED_OPENMP=1
-inherit eutils toolchain-funcs
+inherit toolchain-funcs
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac
if [[ ! ${_FORTRAN_2_CLASS} ]]; then
# Set to "1" in order to automatically have the eclass abort if the fortran
# compiler lacks openmp support.
: ${FORTRAN_NEED_OPENMP:=0}
# Set this, if a special dialect needs to be supported.
# Generally not needed as default is sufficient.
#
# Valid settings are any combination of: 77 90 95 2003
: ${FORTRAN_STANDARD:=77}
# If your package has an optional fortran support, set this variable
# to the space separated list of USE triggering the fortran
# dependency.
#
# e.g. FORTRAN_NEEDED=lapack would result in
#
# DEPEND="lapack? ( virtual/fortran )"
#
# If unset, we always depend on virtual/fortran.
: ${FORTRAN_NEEDED:=always}
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
DEPEND+=" virtual/fortran"
RDEPEND+=" virtual/fortran"
break
;;
no)
break
;;
test)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
*)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
RDEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
esac
done
unset _f_use
# Return the Fortran compiler flag to enable 64 bit integers for
# array indices
fortran_int64_abi_fflags() {
_FC=$(tc-getFC)
Any reason not to make it local?
Post by Andrew Savchenko
if [[ ${_FC} == *gfortran* ]]; then
echo "-fdefault-integer-8"
elif [[ ${_FC} == ifort ]]; then
echo "-integer-size 64"
else
die "Compiler flag for 64bit interger for ${_FC} unknown"
fi
}
# writes fortran test code
_fortran_write_testsuite() {
local filebase=${T}/test-fortran
# f77 code
cat <<- EOF > "${filebase}.f"
|| die
Post by Andrew Savchenko
end
EOF
# f90/95 code
cat <<- EOF > "${filebase}.f90"
|| die
Post by Andrew Savchenko
end
Also, why different indentation?
Post by Andrew Savchenko
EOF
# f2003 code
cat <<- EOF > "${filebase}.f03"
|| die
Post by Andrew Savchenko
procedure(), pointer :: p
end
EOF
}
# Takes fortran compiler as first argument and dialect as second.
# Checks whether the passed fortran compiler speaks the fortran dialect
_fortran_compile_test() {
local filebase=${T}/test-fortran
local fcomp=${1}
local fdia=${2}
local fcode=${filebase}.f${fdia}
local ret
[[ $# -lt 1 ]] && \
die "_fortran_compile_test() needs at least one argument"
[[ -f ${fcode} ]] || _fortran_write_testsuite
${fcomp} "${fcode}" -o "${fcode}.x" \
Post by Michał Górny
Post by Andrew Savchenko
"${T}"/_fortran_compile_test.log 2>&1
ret=$?
rm -f "${fcode}.x"
return ${ret}
}
# See if the fortran supports OpenMP.
_fortran-has-openmp() {
local flag
local filebase=${T}/test-fc-openmp
local fcode=${filebase}.f
local ret
local _fc=$(tc-getFC)
cat <<- EOF > "${fcode}"
|| die
Post by Andrew Savchenko
call omp_get_num_threads
end
EOF
for flag in -fopenmp -xopenmp -openmp -mp -omp -qsmp=omp; do
${_fc} ${flag} "${fcode}" -o "${fcode}.x" \
&>> "${T}"/_fortran_compile_test.log
ret=$?
(( ${ret} )) || break
This (( ... )) is unreadable at best; please replace it with clear
condition.
Post by Andrew Savchenko
done
rm -f "${fcode}.x"
return ${ret}
}
# Detailed description how to handle fortran support
_fortran_die_msg() {
echo
Don't mix echo with eerror.
Post by Andrew Savchenko
eerror "Please install currently selected gcc version with USE=fortran."
eerror "If you intend to use a different compiler then gfortran, please"
eerror "set FC variable accordingly and take care that the necessary"
eerror "fortran dialects are supported."
echo
die "Currently no working fortran compiler is available (see ${T}/_fortran_compile_test.log for information)"
}
# Internal test function for working fortran compiler.
# It is called in fortran-2_pkg_setup.
_fortran_test_function() {
local dialect
: ${F77:=$(tc-getFC)}
: ${FORTRAN_STANDARD:=77}
for dialect in ${FORTRAN_STANDARD}; do
case ${dialect} in
77) _fortran_compile_test $(tc-getF77) || \
_fortran_die_msg ;;
90|95) _fortran_compile_test $(tc-getFC) 90 || \
_fortran_die_msg ;;
2003) _fortran_compile_test $(tc-getFC) 03 || \
_fortran_die_msg ;;
2008) die "Future" ;;
*) die "${dialect} is not a Fortran dialect." ;;
esac
done
tc-export F77 FC
einfo "Using following Fortran compiler:"
einfo " F77: ${F77}"
einfo " FC: ${FC}"
if [[ ${FORTRAN_NEED_OPENMP} == 1 ]]; then
if _fortran-has-openmp; then
einfo "${FC} has OPENMP support"
else
die "Please install current gcc with USE=openmp or set the FC variable to a compiler that supports OpenMP"
fi
fi
}
# _The_ fortran-2_pkg_setup() code
_fortran-2_pkg_setup() {
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
_fortran_test_function && break
;;
no)
einfo "Forcing fortran support off"
break
;;
*)
if use ${_f_use}; then
_fortran_test_function && break
else
unset FC
unset F77
fi
This contradicts the dependency atoms.

If FORTRAN_NEEDED="foo bar", you'll get:

DEP="foo? ( virtual/fortran ) bar? ( virtual/fortran )"

However, with USE="foo -bar" this will first set the compiler
for USE=foo, then reset it for USE=bar.
Post by Andrew Savchenko
;;
esac
done
}
# Setup functionality,
# checks for a valid fortran compiler and optionally for its openmp support.
fortran-2_pkg_setup() {
if [[ ${MERGE_TYPE} != binary ]]; then
_fortran-2_pkg_setup
fi
}
_FORTRAN_2_ECLASS=1
fi
--
Best regards,
Michał Górny
Andrew Savchenko
2018-11-01 22:27:44 UTC
Permalink
Hi!
Post by Michał Górny
Post by Andrew Savchenko
Post by Michał Górny
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
I'm going to include a few 'easy cleanup' comments since EAPI 7
is a good opportunity to improve the eclass. I'm going to skip horribly
bad design decisions since I suppose nobody cares.
Should we really mix EAPI bump with full code review?

This eclass is small, so no harm here. But for larger eclasses
(hello java-*.eclass) this will hinder updates considerably. I
prefer to fix something rather than to fix nothing while
frustrating in attempt to fix everything at once.

Also this make git history review harder as fixes for independent
issues will be mixed together.

So I kindly ask you for future updates (from everyone, not just
me) focus on review of the proposed changes instead of reviewing
full code. Thank you for understanding.
Post by Michał Górny
Post by Andrew Savchenko
# Return the Fortran compiler flag to enable 64 bit integers for
# array indices
fortran_int64_abi_fflags() {
_FC=$(tc-getFC)
Any reason not to make it local?
Fixed.
Post by Michał Górny
Post by Andrew Savchenko
# writes fortran test code
_fortran_write_testsuite() {
local filebase=${T}/test-fortran
# f77 code
cat <<- EOF > "${filebase}.f"
|| die
Done.
Post by Michał Górny
Post by Andrew Savchenko
end
EOF
# f90/95 code
cat <<- EOF > "${filebase}.f90"
|| die
Done.
Post by Michał Górny
Post by Andrew Savchenko
end
Also, why different indentation?
I prefer not to touch it. Fortran compilers are quite picky with
leading spaces or tabs.
Post by Michał Górny
Post by Andrew Savchenko
EOF
# f2003 code
cat <<- EOF > "${filebase}.f03"
|| die
Done.
Post by Michał Górny
Post by Andrew Savchenko
# See if the fortran supports OpenMP.
_fortran-has-openmp() {
local flag
local filebase=${T}/test-fc-openmp
local fcode=${filebase}.f
local ret
local _fc=$(tc-getFC)
cat <<- EOF > "${fcode}"
|| die
Done.
Post by Michał Górny
Post by Andrew Savchenko
for flag in -fopenmp -xopenmp -openmp -mp -omp -qsmp=omp; do
${_fc} ${flag} "${fcode}" -o "${fcode}.x" \
&>> "${T}"/_fortran_compile_test.log
ret=$?
(( ${ret} )) || break
This (( ... )) is unreadable at best; please replace it with clear
condition.
Fixed. ret variable is not needed at all.
Post by Michał Górny
Post by Andrew Savchenko
# Detailed description how to handle fortran support
_fortran_die_msg() {
echo
Don't mix echo with eerror.
Done.
Post by Michał Górny
Post by Andrew Savchenko
# _The_ fortran-2_pkg_setup() code
_fortran-2_pkg_setup() {
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
_fortran_test_function && break
;;
no)
einfo "Forcing fortran support off"
break
;;
*)
if use ${_f_use}; then
_fortran_test_function && break
else
unset FC
unset F77
fi
This contradicts the dependency atoms.
DEP="foo? ( virtual/fortran ) bar? ( virtual/fortran )"
However, with USE="foo -bar" this will first set the compiler
for USE=foo, then reset it for USE=bar.
Ok, now both case and for will break immediately if fortran
compiler is found and passed tests.

The updated full v2 patch will be sent as a separate e-mail.

Best regards,
Andrew Savchenko
Michael Orlitzky
2018-11-02 00:47:54 UTC
Permalink
Post by Andrew Savchenko
This eclass is small, so no harm here. But for larger eclasses
(hello java-*.eclass) this will hinder updates considerably. I
prefer to fix something rather than to fix nothing while
frustrating in attempt to fix everything at once.
Also this make git history review harder as fixes for independent
issues will be mixed together.
So I kindly ask you for future updates (from everyone, not just
me) focus on review of the proposed changes instead of reviewing
full code. Thank you for understanding.
You don't have to fix everything at once. A thorough code review is
incredibly valuable we shouldn't discourage anyone from doing them.

On the other hand, if you decide to fix only some of the issues, that's
your prerogative. I would however suggest that afterwards, you open a
bug for the remaining improvements so that the valuable time of the
reviewer is not wasted.
Michał Górny
2018-11-02 14:20:16 UTC
Permalink
Post by Andrew Savchenko
Hi!
Post by Michał Górny
Post by Michał Górny
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
I'm going to include a few 'easy cleanup' comments since EAPI 7
is a good opportunity to improve the eclass. I'm going to skip horribly
bad design decisions since I suppose nobody cares.
Should we really mix EAPI bump with full code review?
Yes, for two reasons.

Firstly, because an EAPI bump effectively requires reviewing all
the eclass logic for constraints imposed by the new EAPI. While
reviewing code, it is natural that people may notice other issues.
Ignoring them once noticed would be a waste of effort.

Secondly, changes to frequently used eclass have a large overhead of
metadata cache updates. Given most of the listed issues are rather
trivial to fix, it would be wasteful to defer them for a second metadata
cache update.
Post by Andrew Savchenko
This eclass is small, so no harm here. But for larger eclasses
(hello java-*.eclass) this will hinder updates considerably. I
prefer to fix something rather than to fix nothing while
frustrating in attempt to fix everything at once.
Also this make git history review harder as fixes for independent
issues will be mixed together.
Why would you mix them together? The whole point of using git (and not
CVS) is that you can trivially make separate commits addressing
different kinds of issues. It also makes it trivial to send them for
review afterwards.
Post by Andrew Savchenko
So I kindly ask you for future updates (from everyone, not just
me) focus on review of the proposed changes instead of reviewing
full code. Thank you for understanding.
As explained above, the proposed change is meaningless without context
(as it affects how everything else in eclass works). If we were to
ignore context, we'd even ACK eclass changes that resulted in the eclass
immediately dying due to programmer's mistake.

Finally, I'd like to point out that peer review is one of foundations
of open source. Sadly, Gentoo has failed to embrace this, and right now reviews of existing code are rather an exception than a rule. What
makes it even worse is that some developers are actively hostile to
the criticism of their code. It is as if Gentoo's bazaar was dominated by makeshift cathedrals.
--
Best regards,
Michał Górny
Andrew Savchenko
2018-11-05 15:37:10 UTC
Permalink
Post by Michał Górny
Post by Andrew Savchenko
Hi!
Post by Michał Górny
Post by Michał Górny
Post by Andrew Savchenko
Hi all!
The only blocker for EAPI 7 update is eutils inheritance, but it
seems to be not used within the current eclass code, probably a
remnant from older days. So it is removed.
Looks like no other EAPI 7 specific changes needed.
Please use -U99999 to include more context to the patches.
I'm going to include a few 'easy cleanup' comments since EAPI 7
is a good opportunity to improve the eclass. I'm going to skip horribly
bad design decisions since I suppose nobody cares.
Should we really mix EAPI bump with full code review?
Yes, for two reasons.
Firstly, because an EAPI bump effectively requires reviewing all
the eclass logic for constraints imposed by the new EAPI. While
reviewing code, it is natural that people may notice other issues.
Ignoring them once noticed would be a waste of effort.
EAPI update usually don't affect full ebuild scope. For EAPI 6->7
update grep is sufficient to find affected parts of the eclass.
Post by Michał Górny
Secondly, changes to frequently used eclass have a large overhead of
metadata cache updates. Given most of the listed issues are rather
trivial to fix, it would be wasteful to defer them for a second metadata
cache update.
Not all eclasses are frequently used, including fortran-2 eclass.
Post by Michał Górny
Post by Andrew Savchenko
So I kindly ask you for future updates (from everyone, not just
me) focus on review of the proposed changes instead of reviewing
full code. Thank you for understanding.
As explained above, the proposed change is meaningless without context
(as it affects how everything else in eclass works). If we were to
ignore context, we'd even ACK eclass changes that resulted in the eclass
immediately dying due to programmer's mistake.
Finally, I'd like to point out that peer review is one of foundations
of open source.
While peer review is important in many cases, the statement above
is wrong. The free software (and not just open source and Gentoo
is free software) founds on the four freedoms as defined by FSF.
Post by Michał Górny
Sadly, Gentoo has failed to embrace this, and right now reviews
of existing code are rather an exception than a rule. What
makes it even worse is that some developers are actively hostile to
the criticism of their code.
Everything is good only within sane limits. While peer review is
often useful, it may be harmful as well:

1) It at least doubles human time required to do the job. And human
resources are the only resources we are really short of.

2) Sometimes it turns into bikeshedding, e.g. of how variables
should be named or what indentation style should be used.

3) Unreasonably long and complicated process for routine changes
may discourage people from further contributions. Right now we have
dozens of eclasses still on EAPI 6. I would prefer to see them
EAPI 7 so they would not block EAPI 7 updates of dependent packages,
rather than require them to underdo complete overhaul during EAPI 7
update effectively postponing it for a long time.

Sometimes the best is the enemy of the good.

Best regards,
Andrew Savchenko
Andreas K. Huettel
2018-10-29 22:52:31 UTC
Permalink
Post by Andrew Savchenko
-inherit eutils toolchain-funcs
+inherit toolchain-funcs
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac
^ The disadvantage of this is that eutils inheritance then suddenly disappears
for all ebuilds. And you don't know who assumed implicitly somewhere that
inheriting fortran-2 makes epatch available...

So how about still inheriting eutils for EAPI 4,5,6 and only dropping it for
EAPI 7 ?!
--
Andreas K. Hüttel
***@gentoo.org
Gentoo Linux developer
(council, toolchain, perl, libreoffice, comrel)
Andrew Savchenko
2018-11-01 22:26:42 UTC
Permalink
Hi!
Post by Andreas K. Huettel
Post by Andrew Savchenko
-inherit eutils toolchain-funcs
+inherit toolchain-funcs
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac
^ The disadvantage of this is that eutils inheritance then suddenly disappears
for all ebuilds. And you don't know who assumed implicitly somewhere that
inheriting fortran-2 makes epatch available...
So how about still inheriting eutils for EAPI 4,5,6 and only dropping it for
EAPI 7 ?!
I agree, backward compatibility matters. Fixed in v2 patch.


Best regards,
Andrew Savchenko
Andrew Savchenko
2018-11-01 22:25:20 UTC
Permalink
Hi all!

Here follows the updated version with improvements proposed by
mgorny and dilfridge.

diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..a0243714b674 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -1,285 +1,289 @@
-# Copyright 1999-2017 Gentoo Foundation
+# Copyright 1999-2018 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

# @ECLASS: fortran-2.eclass
# @MAINTAINER:
# ***@gentoo.org
# ***@gentoo.org
# @AUTHOR:
# Author Justin Lecher <***@gentoo.org>
# Test functions provided by Sebastien Fabbro and Kacper Kowalik
-# @SUPPORTED_EAPIS: 4 5 6
+# @SUPPORTED_EAPIS: 4 5 6 7
# @BLURB: Simplify fortran compiler management
# @DESCRIPTION:
# If you need a fortran compiler, then you should be inheriting this eclass.
# In case you only need optional support, please export FORTRAN_NEEDED before
# inheriting the eclass.
#
# The eclass tests for working fortran compilers
# and exports the variables FC and F77.
# Optionally, it checks for extended capabilities based on
# the variable options selected in the ebuild
# The only phase function exported is fortran-2_pkg_setup.
# @EXAMPLE:
# FORTRAN_NEEDED="lapack fortran"
#
# inherit fortran-2
#
# FORTRAN_NEED_OPENMP=1

-inherit eutils toolchain-funcs
+inherit toolchain-funcs
+case ${EAPI:-0} in
+ # not used in the eclass, but left for backward compatibility with legacy users
+ 4|5|6) inherit eutils ;;
+ 7) ;;
+ *) die "EAPI=${EAPI} is not supported" ;;
+esac

case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac

if [[ ! ${_FORTRAN_2_CLASS} ]]; then

# @ECLASS-VARIABLE: FORTRAN_NEED_OPENMP
# @DESCRIPTION:
# Set to "1" in order to automatically have the eclass abort if the fortran
# compiler lacks openmp support.
: ${FORTRAN_NEED_OPENMP:=0}

# @ECLASS-VARIABLE: FORTRAN_STANDARD
# @DESCRIPTION:
# Set this, if a special dialect needs to be supported.
# Generally not needed as default is sufficient.
#
# Valid settings are any combination of: 77 90 95 2003
: ${FORTRAN_STANDARD:=77}

# @ECLASS-VARIABLE: FORTRAN_NEEDED
# @DESCRIPTION:
# If your package has an optional fortran support, set this variable
# to the space separated list of USE triggering the fortran
# dependency.
#
# e.g. FORTRAN_NEEDED=lapack would result in
#
# DEPEND="lapack? ( virtual/fortran )"
#
# If unset, we always depend on virtual/fortran.
: ${FORTRAN_NEEDED:=always}

for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
DEPEND+=" virtual/fortran"
RDEPEND+=" virtual/fortran"
break
;;
no)
break
;;
test)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
*)
DEPEND+=" ${_f_use}? ( virtual/fortran )"
RDEPEND+=" ${_f_use}? ( virtual/fortran )"
;;
esac
done
unset _f_use

# @FUNCTION: fortran_int64_abi_fflags
# @DESCRIPTION:
# Return the Fortran compiler flag to enable 64 bit integers for
# array indices
# @CODE
fortran_int64_abi_fflags() {
debug-print-function ${FUNCNAME} "${@}"

- _FC=$(tc-getFC)
+ local _FC=$(tc-getFC)
if [[ ${_FC} == *gfortran* ]]; then
echo "-fdefault-integer-8"
elif [[ ${_FC} == ifort ]]; then
echo "-integer-size 64"
else
die "Compiler flag for 64bit interger for ${_FC} unknown"
fi
}

# @FUNCTION: _fortran_write_testsuite
# @INTERNAL
# @DESCRIPTION:
# writes fortran test code
_fortran_write_testsuite() {
debug-print-function ${FUNCNAME} "${@}"

local filebase=${T}/test-fortran

# f77 code
- cat <<- EOF > "${filebase}.f"
+ cat <<- EOF > "${filebase}.f" || die
end
EOF

# f90/95 code
- cat <<- EOF > "${filebase}.f90"
+ cat <<- EOF > "${filebase}.f90" || die
end
EOF

# f2003 code
- cat <<- EOF > "${filebase}.f03"
+ cat <<- EOF > "${filebase}.f03" || die
procedure(), pointer :: p
end
EOF
}

# @FUNCTION: _fortran_compile_test
# @USAGE: <compiler> [dialect]
# @INTERNAL
# @DESCRIPTION:
# Takes fortran compiler as first argument and dialect as second.
# Checks whether the passed fortran compiler speaks the fortran dialect
_fortran_compile_test() {
debug-print-function ${FUNCNAME} "${@}"

local filebase=${T}/test-fortran
local fcomp=${1}
local fdia=${2}
local fcode=${filebase}.f${fdia}
local ret

[[ $# -lt 1 ]] && \
die "_fortran_compile_test() needs at least one argument"

[[ -f ${fcode} ]] || _fortran_write_testsuite

${fcomp} "${fcode}" -o "${fcode}.x" \
Post by Andrew Savchenko
"${T}"/_fortran_compile_test.log 2>&1
ret=$?

rm -f "${fcode}.x"
return ${ret}
}

# @FUNCTION: _fortran-has-openmp
# @RETURN: return code of the compiler
# @INTERNAL
# @DESCRIPTION:
# See if the fortran supports OpenMP.
_fortran-has-openmp() {
debug-print-function ${FUNCNAME} "${@}"

local flag
local filebase=${T}/test-fc-openmp
local fcode=${filebase}.f
- local ret
local _fc=$(tc-getFC)

- cat <<- EOF > "${fcode}"
+ cat <<- EOF > "${fcode}" || die
call omp_get_num_threads
end
EOF

for flag in -fopenmp -xopenmp -openmp -mp -omp -qsmp=omp; do
${_fc} ${flag} "${fcode}" -o "${fcode}.x" \
&>> "${T}"/_fortran_compile_test.log
- ret=$?
- (( ${ret} )) || break
+ [[ $? == 0 ]] && break
done

rm -f "${fcode}.x"
return ${ret}
}

# @FUNCTION: _fortran_die_msg
# @INTERNAL
# @DESCRIPTION:
# Detailed description how to handle fortran support
_fortran_die_msg() {
debug-print-function ${FUNCNAME} "${@}"

- echo
+ eerror
eerror "Please install currently selected gcc version with USE=fortran."
eerror "If you intend to use a different compiler then gfortran, please"
eerror "set FC variable accordingly and take care that the necessary"
eerror "fortran dialects are supported."
- echo
+ eerror
die "Currently no working fortran compiler is available (see ${T}/_fortran_compile_test.log for information)"
}

# @FUNCTION: _fortran_test_function
# @INTERNAL
# @DESCRIPTION:
# Internal test function for working fortran compiler.
# It is called in fortran-2_pkg_setup.
_fortran_test_function() {
debug-print-function ${FUNCNAME} "${@}"

local dialect

: ${F77:=$(tc-getFC)}

: ${FORTRAN_STANDARD:=77}
for dialect in ${FORTRAN_STANDARD}; do
case ${dialect} in
77) _fortran_compile_test $(tc-getF77) || \
_fortran_die_msg ;;
90|95) _fortran_compile_test $(tc-getFC) 90 || \
_fortran_die_msg ;;
2003) _fortran_compile_test $(tc-getFC) 03 || \
_fortran_die_msg ;;
2008) die "Future" ;;
*) die "${dialect} is not a Fortran dialect." ;;
esac
done

tc-export F77 FC
einfo "Using following Fortran compiler:"
einfo " F77: ${F77}"
einfo " FC: ${FC}"

if [[ ${FORTRAN_NEED_OPENMP} == 1 ]]; then
if _fortran-has-openmp; then
einfo "${FC} has OPENMP support"
else
die "Please install current gcc with USE=openmp or set the FC variable to a compiler that supports OpenMP"
fi
fi
}

# @FUNCTION: _fortran-2_pkg_setup
# @INTERNAL
# @DESCRIPTION:
# _The_ fortran-2_pkg_setup() code
_fortran-2_pkg_setup() {
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
- _fortran_test_function && break
+ _fortran_test_function && break 2
;;
no)
einfo "Forcing fortran support off"
break
;;
*)
if use ${_f_use}; then
- _fortran_test_function && break
+ _fortran_test_function && break 2
else
unset FC
unset F77
fi
;;
esac
done
}


# @FUNCTION: fortran-2_pkg_setup
# @DESCRIPTION:
# Setup functionality,
# checks for a valid fortran compiler and optionally for its openmp support.
fortran-2_pkg_setup() {
debug-print-function ${FUNCNAME} "${@}"

if [[ ${MERGE_TYPE} != binary ]]; then
_fortran-2_pkg_setup
fi
}

_FORTRAN_2_ECLASS=1
fi


Best regards,
Andrew Savchenko
Ulrich Mueller
2018-11-02 10:27:29 UTC
Permalink
Post by Andrew Savchenko
-inherit eutils toolchain-funcs
+inherit toolchain-funcs
+case ${EAPI:-0} in
+ # not used in the eclass, but left for backward compatibility with legacy users
+ 4|5|6) inherit eutils ;;
+ 7) ;;
+ *) die "EAPI=${EAPI} is not supported" ;;
+esac
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac
Why is the second case statement needed? Program flow can only reach it
if the EAPI is 4, 5, 6, or 7.

Ulrich
Andrew Savchenko
2018-11-05 14:30:13 UTC
Permalink
Post by Ulrich Mueller
Post by Andrew Savchenko
-inherit eutils toolchain-funcs
+inherit toolchain-funcs
+case ${EAPI:-0} in
+ # not used in the eclass, but left for backward compatibility with legacy users
+ 4|5|6) inherit eutils ;;
+ 7) ;;
+ *) die "EAPI=${EAPI} is not supported" ;;
+esac
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ 4|5|6|7) EXPORT_FUNCTIONS pkg_setup ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac
Why is the second case statement needed? Program flow can only reach it
if the EAPI is 4, 5, 6, or 7.
Just a remnant from older version. Fixed.

Best regards,
Andrew Savchenko
Andrew Savchenko
2018-11-05 15:38:08 UTC
Permalink
Hi all!

Here follow updated patches for fortran-2.eclass EAPI 7 update.

Patch 1 contains only EAPI 7 related changes:

diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..b871d16e3e05 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -8,7 +8,7 @@
# @AUTHOR:
# Author Justin Lecher <***@gentoo.org>
# Test functions provided by Sebastien Fabbro and Kacper Kowalik
-# @SUPPORTED_EAPIS: 4 5 6
+# @SUPPORTED_EAPIS: 4 5 6 7
# @BLURB: Simplify fortran compiler management
# @DESCRIPTION:
# If you need a fortran compiler, then you should be inheriting
this eclass. @@ -27,13 +27,16 @@
#
# FORTRAN_NEED_OPENMP=1

-inherit eutils toolchain-funcs
-
+inherit toolchain-funcs
case ${EAPI:-0} in
- 4|5|6) EXPORT_FUNCTIONS pkg_setup ;;
+ # not used in the eclass, but left for backward
compatibility with legacy users
+ 4|5|6) inherit eutils ;;
+ 7) ;;
*) die "EAPI=${EAPI} is not supported" ;;
esac

+EXPORT_FUNCTIONS pkg_setup
+
if [[ ! ${_FORTRAN_2_CLASS} ]]; then

# @ECLASS-VARIABLE: FORTRAN_NEED_OPENMP


Best regards,
Andrew Savchenko
Andrew Savchenko
2018-11-05 15:37:55 UTC
Permalink
Hi all!

Here follow updated patches for fortran-2.eclass EAPI 7 update.

Patch 2 contains only code cleanup and fixes unrelated to EAPI 7
update:

diff --git a/eclass/fortran-2.eclass b/eclass/fortran-2.eclass
index 820cbbcb49bd..b871d16e3e05 100644
--- a/eclass/fortran-2.eclass
+++ b/eclass/fortran-2.eclass
@@ -1,4 +1,4 @@
-# Copyright 1999-2017 Gentoo Foundation
+# Copyright 1999-2018 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

# @ECLASS: fortran-2.eclass
@@ -92,7 +95,7 @@ unset _f_use
fortran_int64_abi_fflags() {
debug-print-function ${FUNCNAME} "${@}"

- _FC=$(tc-getFC)
+ local _FC=$(tc-getFC)
if [[ ${_FC} == *gfortran* ]]; then
echo "-fdefault-integer-8"
elif [[ ${_FC} == ifort ]]; then
@@ -112,17 +115,17 @@ _fortran_write_testsuite() {
local filebase=${T}/test-fortran

# f77 code
- cat <<- EOF > "${filebase}.f"
+ cat <<- EOF > "${filebase}.f" || die
end
EOF

# f90/95 code
- cat <<- EOF > "${filebase}.f90"
+ cat <<- EOF > "${filebase}.f90" || die
end
EOF

# f2003 code
- cat <<- EOF > "${filebase}.f03"
+ cat <<- EOF > "${filebase}.f03" || die
procedure(), pointer :: p
end
EOF
@@ -170,7 +173,7 @@ _fortran-has-openmp() {
local ret
local _fc=$(tc-getFC)

- cat <<- EOF > "${fcode}"
+ cat <<- EOF > "${fcode}" || die
call omp_get_num_threads
end
EOF
@@ -179,7 +182,7 @@ _fortran-has-openmp() {
${_fc} ${flag} "${fcode}" -o "${fcode}.x" \
&>> "${T}"/_fortran_compile_test.log
ret=$?
- (( ${ret} )) || break
+ [[ ${ret} == 0 ]] && break
done

rm -f "${fcode}.x"
@@ -193,12 +196,12 @@ _fortran-has-openmp() {
_fortran_die_msg() {
debug-print-function ${FUNCNAME} "${@}"

- echo
+ eerror
eerror "Please install currently selected gcc version with USE=fortran."
eerror "If you intend to use a different compiler then gfortran, please"
eerror "set FC variable accordingly and take care that the necessary"
eerror "fortran dialects are supported."
- echo
+ eerror
die "Currently no working fortran compiler is available (see ${T}/_fortran_compile_test.log for information)"
}

@@ -250,7 +253,7 @@ _fortran-2_pkg_setup() {
for _f_use in ${FORTRAN_NEEDED}; do
case ${_f_use} in
always)
- _fortran_test_function && break
+ _fortran_test_function && break 2
;;
no)
einfo "Forcing fortran support off"
@@ -258,7 +261,7 @@ _fortran-2_pkg_setup() {
;;
*)
if use ${_f_use}; then
- _fortran_test_function && break
+ _fortran_test_function && break 2
else
unset FC
unset F77


Best regards,
Andrew Savchenko
Andrew Savchenko
2018-11-17 11:38:39 UTC
Permalink
Post by Andrew Savchenko
Hi all!
Here follow updated patches for fortran-2.eclass EAPI 7 update.
Patch 2 contains only code cleanup and fixes unrelated to EAPI 7
With no comments for ~12 days both patches are applied now.

Best regards,
Andrew Savchenko

Loading...