Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-3749

Scons build of lapack_functions in PSFex fails if SCONSFLAGS are set

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: psfex
    • Labels:
      None

      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

        Attachments

          Issue Links

            Activity

            Hide
            swinbank 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
            swinbank 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
            swinbank 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
            swinbank 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
            price Paul Price added a comment -

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

            Show
            price Paul Price added a comment - I agree with the proposed solution; haven't looked at the actual changes though.
            Hide
            nlust 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
            nlust 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
            swinbank 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
            swinbank 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:
                swinbank John Swinbank
                Reporter:
                nlust Nate Lust
                Reviewers:
                Kian-Tat Lim, Nate Lust, Tim Jenness
                Watchers:
                John Swinbank, Joshua Hoblitt, Nate Lust, Paul Price, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: