Re: [gentoo-dev] [PATCH] fortran-2.eclass: support EAPI 7

[Andrew Savchenko <bircoph@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 4520 bytes --]

Hi!

On Tue, 30 Oct 2018 08:18:58 +0100 Michał Górny wrote:
> On Mon, 2018-10-29 at 03:57 +0300, Andrew Savchenko wrote:
> > On Sun, 28 Oct 2018 19:29:28 +0100 Michał Górny wrote:
> > > On Sun, 2018-10-28 at 01:38 +0300, Andrew Savchenko wrote:
> > > > 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.

> >  # @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)
> 
> Any reason not to make it local?

Fixed.
 
> >  # @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"
> 
> || die

Done.

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

Done.

> >  	end
> 
> Also, why different indentation?

I prefer not to touch it. Fortran compilers are quite picky with
leading spaces or tabs.

> >  	EOF
> >  
> >  	# f2003 code
> >  	cat <<- EOF > "${filebase}.f03"
> 
> || die

Done.

> >  # @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}"
> 
> || die

Done.
 
> >  	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.

> >  # @FUNCTION: _fortran_die_msg
> >  # @INTERNAL
> >  # @DESCRIPTION:
> >  # Detailed description how to handle fortran support
> >  _fortran_die_msg() {
> >  	debug-print-function ${FUNCNAME} "${@}"
> >  
> >  	echo
> 
> Don't mix echo with eerror.

Done.

> >  # @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
> 
> 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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]