Fix Version/s: None
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
|Field||Original Value||New Value|
|Team||Data Release Production [ 10301 ]|
|Assignee||Nate Lust [ nlust ]|
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.
|Reviewers||Robert Lupton [ rhl ]|
|Status||To Do [ 10001 ]||In Review [ 10004 ]|
I talked with @rhl who is short on time for reviewing this. I'll attempt to look at it this afternoon.
|Reviewers||Robert Lupton [ rhl ]||John Swinbank [ swinbank ]|
Robert Lupton is $SCONSFLAGS being set as a side effect of an operation in sconsUtils? AFAIK - it is not being set directly.
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.
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.
|Reviewers||John Swinbank [ swinbank ]||Kian-Tat Lim, Tim Jenness [ ktl, tjenness ]|
|Assignee||Nate Lust [ nlust ]||John Swinbank [ swinbank ]|
|Reviewers||Kian-Tat Lim, Tim Jenness [ ktl, tjenness ]||Kian-Tat Lim, Nate Lust, Tim Jenness [ ktl, nlust, tjenness ]|
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.
I agree with the proposed solution; haven't looked at the actual changes though.
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
|Status||In Review [ 10004 ]||Reviewed [ 10101 ]|
|Resolution||Done [ 10000 ]|
|Status||Reviewed [ 10101 ]||Done [ 10002 ]|
More precisely, if $SCONSFLAGS (which scons obeys) includes flags added by sconsUtils.