Make sure distributed libraries are relocatable

[Robbybp]

• Use the --enable-relocatable flag when configuing Ipopt to make sure the ipopt.pc file uses relative paths
• Add an install_name_tool step on Darwin to define library names with @rpath
• Compress the idaes-local tar file after install names have been updated (only relevant for Darwin)

With these changes, I believe a user will still need the following steps to link against our IDAES libraries:

• export PKG_CONFIG_PATH="${PKG_CONFIG_PATH}:$HOME/.idaes/lib/pkgconfig"
• export DYLD_LIBRARY_PATH="${DYLD_LIBRARY_PATH}:/$HOME/.idaes/lib
• Patch .idaes/lib/pkgconfig/ipopt.pc to comment out the Requires.private: coinhsl coinmumps line

We could apply a patch in compile_solvers.sh to comment out the "Requires.private" line in ipopt.pc, but it would be nice if we could somehow compile Ipopt such that these requirements (which don't seem to actually be necessary) are not included.

[Robbybp]

it would be nice if we could somehow compile Ipopt such that these requirements (which don't seem to actually be necessary) are not included.

Maybe these libraries actually are necessary... I have been testing on a machine with my own CoinHSL installed. When I temporarily uninstall my own HSL, I get a run-time linker error trying to run CyIpopt, saying it can't find libcoinhsl.2.dylib. I had assumed we were bundling libcoinhsl in with libipopt, but that appears to not be the case (or else my machine still thinks it needs the HSL library).

@jsiirola Can you confirm you were able to run CyIpopt using our libipopt.so on a machine without any other coinhsl installed/discoverable?

[Robbybp]

This PR needs to:

• [x] Update the compile script to patch the ipopt.pc file to remove Requires.private: coinhsl coinmumps
• [x] Update the compile script to compress files with respect to the "top-level" (.idaes) directory. If possible, we should distribute one tar file, possibly idaes-extensions-<os>-<arch>.tar.gz, that contains lib, bin, and include directories, that each contain the expected files.
• [x] Update the mac_collect.sh script. Make sure the resulting tar file for M1 Mac is *-darwin-aarch64.tar.gz

Then we can do the following in parallel:

• Test the build process (on all platforms) with the updated compile script
• Update idaes get-extensions to expect the new directory format of the extensions
• Prototype an idaes install cyipopt command that just sets the required environment variables then calls pip install cyipopt

Additionally, we would like to update the Ipopt we distribute to 3.14.12. Hopefully this is as simple as updating the Ipopt branch in the compile script. This should probably happen in a separate PR, and should probably happen before we publish a new release of the extensions.

[Robbybp]

Maybe these libraries actually are necessary... I have been testing on a machine with my own CoinHSL installed. When I temporarily uninstall my own HSL, I get a run-time linker error trying to run CyIpopt, saying it can't find libcoinhsl.2.dylib. I had assumed we were bundling libcoinhsl in with libipopt, but that appears to not be the case (or else my machine still thinks it needs the HSL library).

I just confirmed that I can run cyipopt with the IDAES-provided libraries, with my own HSL uninstalled. In the previous test that failed, I was linking against libraries that I built locally with the compile_solvers.sh script. These libraries require my own version of HSL to link against, but the IDAES-provided libraries from idaes get-extensions do not. I'm guessing that the libraries we distribute are built against a static coinhsl library.

[adowling2]

@jialuw96 This PR is likely of interest to you!

[Robbybp]

The "Requires.private" line that we need to remove seems to be either Requires.private: coinhsl coinmumps or Requires.private: coinmumps depending on whether or not the Ipopt configure script can find an HSL library. I would expect that coinhsl would be omitted here if we are linking it as a static library, but the distributed ipopt.pc file from idaes get-extensions contains both coinhsl and coinmumps here. If we want to cover either case, a sed command to apply the patch would be pretty simple (although then we might have to handle platform-dependent sed behavior...).

I've inluded a patch to comment out the Requires.private: coinhsl coinmumps line.

[Robbybp]

I've updated the compile script to pack all the solvers (including Petsc) into one tar file, idaes-extensions-<os>-<arch>.tar.gz, which has the following directory structure:

.
├── bin
│   ├── bonmin
│   ├── cbc
│   ├── clp
│   ├── couenne
│   ├── dot_sens
│   ├── ipopt
│   ├── ipopt_l1
│   ├── ipopt_sens
│   ├── ipopt_sens_l1
│   └── k_aug
├── include
│   └── coin-or
│       ├── HSLLoader.h
│       ├── IpAlgBuilder.hpp
│       ├── IpAlgStrategy.hpp
│       ├── IpAlgTypes.hpp
│       ├── IpAugSystemSolver.hpp
│       ...
├── lib
│   ├── libgcc_s.1.1.dylib
│   ├── libgfortran.5.dylib
│   ├── libgomp.1.dylib
│   ├── libipopt.3.dylib
│   ├── libipopt.dylib
│   ...
│   ├── petscpy
│   │   └── petsc_conf.py
│   └── pkgconfig
│       └── ipopt.pc
├── license.txt
├── share
│   └── doc
│       └── ipopt
│           ├── AUTHORS
│           ├── LICENSE
│           └── README.md
└── version_solvers.txt

This will allow us to make some simplifications to the idaes get-extensions command.

I haven't updated the compile_libs.sh script. This should probably be updated to pack the function libraries as e.g. lib/functions.so, instead of including them at the top level of the tar file.

[Robbybp]

I've updated compile_libs.sh to put the function libraries in a lib subdirectory. It now creates a tar file called idaes-functions-*. compile_solvers.sh now creates a tar file called idaes-solvers-*.

[Robbybp]

@bpaul4 @MarcusHolly I've made some significant changes to the compile_solvers.sh and compile_libs.sh scripts, in an attempt to make our distributed libraries more convenient for people to link against (e.g. so we can use them with CyIpopt). When you have the chance, can you test the build process with the new compile scripts? Let me know if you have any questions about the changes I've made.

[ksbeattie]

@bpaul4 & @MarcusHolly, any news on this?

[MarcusHolly]

@bpaul4 & @MarcusHolly, any news on this?

@ksbeattie I think Brandon doesn't have the time atm, and I still need to get my build for the IDAES binaries working, which I've been pushing off in favor of other priorities (that don't require me bashing my head into a wall).

[Robbybp]

From the perspective of the Docker build process, I think the biggest change this PR makes is that it changes the tar files we distribute. Instead of distributing ideas-lib-*.tar.gz, ideas-solvers-*.tar.gz, and idaes-petsc-*.tar.gz, my proposal is to distribute ideas-solvers-*.tar.gz (contains solvers and petsc) and idaes-functions-*.tar.gz. Both of these have updated directory structures, but that should not matter for the Docker build process. This will require changes in the build scripts. I have not tested the full Docker build process myself, but updating the tar files that these scripts expect to find after building is the only obvious required change to the Docker build process that I see so far.

[MarcusHolly]

From the perspective of the Docker build process, I think the biggest change this PR makes is that it changes the tar files we distribute. Instead of distributing ideas-lib-*.tar.gz, ideas-solvers-*.tar.gz, and idaes-petsc-*.tar.gz, my proposal is to distribute ideas-solvers-*.tar.gz (contains solvers and petsc) and idaes-functions-*.tar.gz. Both of these have updated directory structures, but that should not matter for the Docker build process. This will require changes in the build scripts. I have not tested the full Docker build process myself, but updating the tar files that these scripts expect to find after building is the only obvious required change to the Docker build process that I see so far.

@Robbybp I was getting ready to note that there were still 3 tar files being generated instead of the proposed 2 (which you've addressed in your latest commit), but everything seems to be working fine on my end. I'll just re-run the builds to make sure the two expected tar files are generated without any issues now.

[MarcusHolly]

@Robbybp @bpaul4 I'm running into the following error when trying to build el7:

This error was introduced by the latest commit b/c I was able to build el7 this morning before the latest commit. Is this an issue with the code itself or do I just have to do something locally like changing the folder names in my directory?

[Robbybp]

This error was introduced by the latest commit b/c I was able to build el7 this morning before the latest commit. Is this an issue with the code itself or do I just have to do something locally like changing the folder names in my directory?

The files probably have different names than what I assumed in the build scripts? You shouldn't have to do anything manually. Do you know what the tar files are called on el7?

[MarcusHolly]

This error was introduced by the latest commit b/c I was able to build el7 this morning before the latest commit. Is this an issue with the code itself or do I just have to do something locally like changing the folder names in my directory?

The files probably have different names than what I assumed in the build scripts? You shouldn't have to do anything manually. Do you know what the tar files are called on el7?

The names you have in the build script look right to me, unless I'm missing something. Regardless, here are the el7 tar file names that I generated this morning (prior to the latest commit): idaes-lib-el7-x86_64.tar.gz idaes-petsc-el7-x86_64.tar.gz idaes-solvers-el7-x86_64.tar.gz

[Robbybp]

The names you have in the build script look right to me, unless I'm missing something. Regardless, here are the el7 tar file names that I generated this morning (prior to the latest commit)

So is the issue that now the tar files are not getting generated at all?

[MarcusHolly]

The names you have in the build script look right to me, unless I'm missing something. Regardless, here are the el7 tar file names that I generated this morning (prior to the latest commit)

So is the issue that now the tar files are not getting generated at all?

Correct

[Robbybp]

Okay, interesting. Building the tar files should not have been altered at all by my most recent commit, which only changes the cp X . lines after building the files. I'll try to run this on my machine and see what happens.

[Robbybp]

@MarcusHolly To confirm, you are running on Windows, and are running the command ./build.sh el7 x86_64 from git bash?

[MarcusHolly]

@MarcusHolly To confirm, you are running on Windows, and are running the command ./build.sh el7 x86_64 from git bash?

@Robbybp I was running sh build.sh el7 x86_64, and I'm using a Windows computer but set Docker Desktop to Linux mode. Basically, I tried to run the 3 Linux steps under "Building" here: https://github.com/IDAES/idaes-ext/blob/main/doc/build.md

[ksbeattie]

@Robbybp will this make the Nov release? As in ready by the end of the month.

[Robbybp]

@ksbeattie Probably not. This will require some iteration between myself and Marcus. I should have a few days to work on this before the end of the month, but I would not bet on it being finished before then.

[Robbybp]

@MarcusHolly I think the problem you're hitting is a fairly simple oversight on my part: While I updated the docker driver build.sh to expect the tarfiles generated by this branch, I did not update build.sh to actually use this branch. Since we probably want the driver to use https://github.com/idaes/idaes-ext.git and main by default, I've added arguments for the repo URL and branch to build.sh. Can you try running:

sh build.sh el7 x86_64 https://github.com/Robbybp/idaes-ext.git relocatable-libs

[Robbybp]

We will also need to update the build.ps1 script to test this branch for the Windows build. This has two complications:

1. build.ps1 already uses $args[1] as an optional argument. This means it is less trivial to interpret arguments as repo and branch. We probably need to branch on the number of arguments accepted and interpret the last one as buildarg_1 while interpreting the arguments immediately after flavor as repo and branch. I'm open to other suggestions.
2. The OS-specific subdirectories of build-extensions, which seem to only be used by build.ps1, have a hard coded repo=https://github.com/idaes/idaes-ext.git and, confusingly, branch=master (this repository uses main as the default branch...). This shouldn't matter for the Linux directories, as we use build.sh for those. But I am unsure how the Windows build works with this apparent bug in the Dockerfile. This is not something we necessarily need to change (if it works, it works), but just something to be aware of as we test the Windows build.

[MarcusHolly]

@Robbybp I've tried running the build for el7 again after incorporating your latest changes, but I still see the same error as last time. As far as what should be changed in build.ps1, I'm not too familiar with the script itself, but what you are proposing makes sense.

[Robbybp]

Can you email me the full log of the el7 build? Maybe something is going wrong upstream of the final copy. Also, can you check the filesystem of the Docker container? It would be helpful to know whether (a) the tarfiles aren't there at all or (b) the tarfiles are there, but with the wrong names.

[Robbybp]

Do you notice this issue with any of the other Linux builds?

[MarcusHolly]

I've emailed you the output, but this same error pops up for all the Linux builds and for Windows. No tar files are generated in the process either.

[Robbybp]

We get the following message, even though the HSL zip file is in extras:

Verify that there are no error message in the output above.
From https://github.com/coin-or-tools/ThirdParty-Mumps
From https://github.com/idaes/Ipopt
#########################################################################
# Get coinhsl.zip if available                                          #
#########################################################################
#########################################################################
# Thirdparty/ASL                                                        #
#########################################################################
HSL Not Available, BUILDING SOLVERS WITHOUT HSL

I will try to reproduce locally and debug.

[MarcusHolly]

I am able to successfully run and produce the tar files for el7. I'll test that the other linux builds work throughout the day.

Edit: All the linux builds were successful.

[Robbybp]

I just pushed a patch to build.ps1. Can you test this as well?

I didn't try to get fancy with named/optional arguments or anything. The second argument to build.ps1 is still $buildarg_1, while the third and fourth arguments are "$repo" and "$branch". So to test this branch's build, please run:

> .\build.ps1 windows --no-cache https://github.com/Robbybp/idaes-ext.git relocatable-libs

[MarcusHolly]

I just pushed a patch to build.ps1. Can you test this as well?

I didn't try to get fancy with named/optional arguments or anything. The second argument to build.ps1 is still $buildarg_1, while the third and fourth arguments are "$repo" and "$branch". So to test this branch's build, please run:

> .\build.ps1 windows --no-cache https://github.com/Robbybp/idaes-ext.git relocatable-libs

@Robbybp Do you have an idea of how long this run should take? It's been running for about 16 hours, but I'm hesitant to restart the build if this is within reason.

[Robbybp]

@MarcusHolly 16 hours seems excessive, especially since the linux builds take about 30 min each for me. How does the log look? Does it look like it's getting stuck somewhere, or is it continuing to make progress?

[MarcusHolly]

It looks like it's stuck... the logs don't give much information. I recall step 7 taking a long time to run in the past, but I forget how long and I'm also not sure if having the coinhsl.zip adds substantially more time or not.

[Robbybp]

Coinhsl shouldn't add much time. This looks like it is setting up the Docker image based on docker\windows\Dockerfile. Not sure what could be going on here... Does the Windows build work when you omit the repo and branch arguments? I'm a little concerned by these lines:

Step 2/8 : ARG repo=https://github.com/idaes/idaes-ext.git
---> Running in 2b6af9ab93bc
Removing intermediate container 26af9ab93bc
---> dfeea44c168C
step 3/8 : ARG branch=master
---> Running in 5cd60feea3e
Removing intermediate container 5d60f2eea3e

We build the image from the Dockerfile with:

docker build --rm ${buildarg_1} --build-arg repo=${repo} --build-arg branch=${branch} -t ${flavor}_build_itmp .

where repo and branch come from the args to build.ps1. So I would expect that these are setting repo and branch ARGs in the Dockerfile. But based on the log, this seems not to be happening.

If you open up your Docker Desktop while this is running, can you see full log from the commands we're running to set up this image?

[Robbybp]

I've never used Docker on Windows, but I've heard there is some "linux mode". You've switched that mode off, right?

[MarcusHolly]

Docker is set to Windows mode. I don't think I can view the logs from Docker Desktop b/c it just says "analyzing image" indefinitely and I can't click on anything else

[MarcusHolly]

I could try running windows without the repo and branch arguments, but that would mean stopping whatever is going on now. Maybe that is fine tho

[Robbybp]

Yes, this is really excessive, and I assume something is going wrong. At this point, I would try to reproduce what you know has worked in the past.

In you Docker "images" tab, do you see "windows_build_itmp"?

[MarcusHolly]

This is what I see in images. I'll admit that I'm still not too familiar with Docker Desktop, but I don't see "windows_build_itmp" anywhere. But I'll cancel this run and make sure the windows build works without the extra arguments.

[Robbybp]

I don't see "windows_build_itmp" anywhere

That is probably expected, as the build of this image is where we're getting stuck.

[MarcusHolly]

My windows build using the files in main worked in about 75min... I'll retry your branch later tonight and see if it gets stuck at the same spot again

[MarcusHolly]

@Robbybp My windows build works fine now on your branch, so it might've just been a transient issue before.

[Robbybp]

Great, thanks for re-testing! I'll add a few lines to docs/build.md to document how to test the builds with a different branch.

Is there anything else we need to do before merging this PR? How are the builds typically tested?

[MarcusHolly]

The builds are usually just tested through GitHub actions, but I'm a bit foggy on the release workflow as a whole. I can also test builds locally, but not sure if that's recommended over the GitHub testing.

As far as this PR goes, I think it's ready to merge.

[Robbybp]

Just looked at the GHA files, and it looks like the they test using idaes get-extensions, which I assume pull the latest (or some pinned) release. So I guess we'll test when we have a release. I also think this PR is ready. Can you give it an approval?

Maybe I'll give an overview of the changes at next week's dev call, and we can merge it then.

[Robbybp]

I think the next steps after merging this PR are:

1. Get our Ipopt fork updated to 3.14
2. Cut an idaes-ext release
3. Update idaes get-extensions

There is a potentially tricky compatability problem between steps 2 and 3, so we should discuss how to do this with the wider group.

[MarcusHolly]

Sounds like a plan - would this discussion take place during next week's IDAES dev call? I don't usually attend those, but I can show up as needed.

[Robbybp]

Yes, my plan is to discuss at next week's dev call.

[Robbybp]

There is a potentially tricky compatability problem between steps 2 and 3, so we should discuss how to do this with the wider group.

In idaes-pse, we actually pin to a specific binary release via default_binary_release in idaes.config, so compatibility shouldn't be a problem. We can just update the tar files we expect at the same time we update the binary release number. Old versions of idaes will just pull old versions of the binaries. I'd still like to discuss with the group before merging this PR, but this will make updating idaes get-extensions much easier.