# Scons build of lapack_functions in PSFex fails if SCONSFLAGS are set

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
2
• Team:
Data Release Production

#### Description

The scons build system is unaware of extra flags which may be set in SCONSFLAGS environment variable, which are used from scons utils. This will cause the build to fail. The package needs to behave properly and build in the presence of these flags

#### Activity

Hide
Robert Lupton added a comment -

More precisely, if $SCONSFLAGS (which scons obeys) includes flags added by sconsUtils. Show Robert Lupton added a comment - More precisely, if$SCONSFLAGS (which scons obeys) includes flags added by sconsUtils.
Hide
Nate Lust added a comment -

I have introduced a script to run the build process of lapack_functions and filter out any flags which cause the build to fail, notifying the user of which flags were not used.

Show
Nate Lust added a comment - I have introduced a script to run the build process of lapack_functions and filter out any flags which cause the build to fail, notifying the user of which flags were not used.
Hide
John Swinbank added a comment -

I talked with @rhl who is short on time for reviewing this. I'll attempt to look at it this afternoon.

Show
John Swinbank added a comment - I talked with @rhl who is short on time for reviewing this. I'll attempt to look at it this afternoon.
Hide
Joshua Hoblitt added a comment -

Robert Lupton is $SCONSFLAGS being set as a side effect of an operation in sconsUtils? AFAIK - it is not being set directly. Show Joshua Hoblitt added a comment - Robert Lupton is$SCONSFLAGS being set as a side effect of an operation in sconsUtils ? AFAIK - it is not being set directly.
Hide
Robert Lupton added a comment -

No, it's a user configuration option. Look at the scons documentation.

As Jim Bosch points out, it'd have been better to add SCONSUTILSFLAGS rather than co-opt SCONSFLAGS.

Show
Robert Lupton added a comment - No, it's a user configuration option. Look at the scons documentation. As Jim Bosch points out, it'd have been better to add SCONSUTILSFLAGS rather than co-opt SCONSFLAGS.
Hide
John Swinbank added a comment -

I took a look at this issue this evening.

I wonder if the easiest thing to do is actually to avoid the two-stage scons here, and just run everything through a single scons sconsUtils build. To make that work, I eliminated the separate liblapackstub library, and just rolled Nate's LAPACK wrapper into psfex itself. I think this makes the build overall simpler and more reliable, and also fixes the problem reported in this issue. While I was at it, I questioned whether we really want the bin/psfex executable, and discovered that nothing broke if I didn't bother building it.

...but I don't feel that I can review this, since I made many of the changes. Robert Lupton, Paul Price & Nate Lust, you have some prior expertise here – do you have any comments? My changes are on u/swinbank/DM-3749 in psfex.

Show
John Swinbank added a comment - I took a look at this issue this evening. I wonder if the easiest thing to do is actually to avoid the two-stage scons here, and just run everything through a single scons sconsUtils build. To make that work, I eliminated the separate liblapackstub library, and just rolled Nate's LAPACK wrapper into psfex itself. I think this makes the build overall simpler and more reliable, and also fixes the problem reported in this issue. While I was at it, I questioned whether we really want the bin/psfex executable, and discovered that nothing broke if I didn't bother building it. ...but I don't feel that I can review this, since I made many of the changes. Robert Lupton , Paul Price & Nate Lust , you have some prior expertise here – do you have any comments? My changes are on u/swinbank/DM-3749 in psfex .
Hide
John Swinbank added a comment -

Frossie Economou asked K-T and/or Tim to review this; I asked Nate. I think the priority is to get it done soon, since it's the last blocker for the release, so I suggest whoever has time first takes it.

All changes are in u/swinbank/DM-3749 on psfex (NB tickets/DM-3749 contains the earlier work which this supersedes; I'll make sure the ticket branch contains the current version before merging). Worth adding that I added a few extra commits: one with the aim of squashing a compiler warning, and the others just tidying up some slightly messy code that I ran across while taking care of the former.

Show
John Swinbank added a comment - Frossie Economou asked K-T and/or Tim to review this; I asked Nate. I think the priority is to get it done soon, since it's the last blocker for the release, so I suggest whoever has time first takes it. All changes are in u/swinbank/DM-3749 on psfex (NB tickets/ DM-3749 contains the earlier work which this supersedes; I'll make sure the ticket branch contains the current version before merging). Worth adding that I added a few extra commits: one with the aim of squashing a compiler warning, and the others just tidying up some slightly messy code that I ran across while taking care of the former.
Hide
Paul Price added a comment -

I agree with the proposed solution; haven't looked at the actual changes though.

Show
Paul Price added a comment - I agree with the proposed solution; haven't looked at the actual changes though.
Hide
Nate Lust added a comment -

Everything looks good, with two minor points. 1. remove the & from the comments in src/lapack_stub.c 2) change Dir('.').abspath to Dir('#') in SConstruct. Otherwise looks good to merge, great job

Show
Nate Lust added a comment - Everything looks good, with two minor points. 1. remove the & from the comments in src/lapack_stub.c 2) change Dir('.').abspath to Dir('#') in SConstruct. Otherwise looks good to merge, great job
Hide
John Swinbank added a comment -

Thanks for the review. Merged. Nate Lust – I shifted the work you did on this to the u/nlust/DM-3749 branch; up to you whether you want to do something with that or just delete it.

Show
John Swinbank added a comment - Thanks for the review. Merged. Nate Lust – I shifted the work you did on this to the u/nlust/ DM-3749 branch; up to you whether you want to do something with that or just delete it.

#### People

Assignee:
John Swinbank
Reporter:
Nate Lust
Reviewers:
Kian-Tat Lim, Nate Lust, Tim Jenness
Watchers:
John Swinbank, Joshua Hoblitt, Nate Lust, Paul Price, Robert Lupton