Rename cmake files

[hainest]

This was originally part of #1264

Co-authored-by: Jonathan R. Madsen jrmadsen@users.noreply.github.com

Closes #1264

[hainest]

No build issues on Power and arm: https://bottle.cs.wisc.edu/search?dyninst_branch=PR1271

[kupsch]

Looks correct.

[jrmadsen]

It does not make sense that your testing passes here.

In dyninst/testsuite, this line should fail:

https://github.com/dyninst/testsuite/blob/8f653c585a4564f319a0f6bb7f5b06c346e30013/CMakeLists.txt#L22

and in dyninst/external-tests, this line should fail:

https://github.com/dyninst/external-tests/blob/743fe86e6a5f34eac8f34526dfcdef9a603a1984/CMakeLists.txt#L8

because those files do not exist anymore. Are you checking out the PR on top of an existing source tree that already exists in the container? Because that would explain why those files are getting installed (if you look, you will see the old filenames and the new filenames) and you are getting a false positive w.r.t. to passing testing:

  [100%] Linking CXX executable parseThat
  [100%] Built target parseThat
  -- Install configuration: "RelWithDebInfo"
  -- Installing: /opt/dyninst-env/install/dyninst/lib/libdyninstAPI_RT.so.12.1.0
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/libdyninstAPI_RT.so.12.1
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/libdyninstAPI_RT.so
  -- Installing: /opt/dyninst-env/install/dyninst/include/dyninstAPI_RT.h
  -- Installing: /opt/dyninst-env/install/dyninst/include/dyninstRTExport.h
  -- Installing: /opt/dyninst-env/install/dyninst/lib/libdyninstAPI_RT.a
  -- Up-to-date: /opt/dyninst-env/install/dyninst/include/dyninstAPI_RT.h
  -- Up-to-date: /opt/dyninst-env/install/dyninst/include/dyninstRTExport.h
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/warnings.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/optimization.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/platform_windows.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/visibility.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Boost.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindValgrind.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindTBB.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindLibDebuginfod.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindLibElf.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindLibIberty.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindLibDwarf.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindBoost.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindThreadDB.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/DyninstSystemPaths.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/Modules/FindThread_DB.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstConfigVersion.cmake.in
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/ThreadingBuildingBlocks.install.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/ThreadingBuildingBlocks.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/version.h.in
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/platform.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/LanguageStandards.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/LibIberty.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/options.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/shared.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/ElfUtils.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstConfig.cmake.in
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/platform_unix.cmake
  -- Up-to-date: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/cap_arch_def.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstOptimization.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstLibrary.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstPlatform.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstPlatformUnix.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstLanguageStandards.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls/DyninstTBB.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls/DyninstTBBInstall.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls/DyninstElfUtils.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls/DyninstBoost.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/tpls/DyninstLibIberty.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstPlatformWindows.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstOptions.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstVisibility.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstWarnings.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstCapArchDef.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/DyninstSystemPaths.cmake
  -- Installing: /opt/dyninst-env/install/dyninst/lib/cmake/Dyninst/CMakeCache.txt

[jrmadsen]

Ah it's bc of this line here:

dyninst/docker/Dockerfile.test

Line 7 in 231dd17

COPY . /code

which copies over the edits on top of an existing source

[hainest]

I'm bringing these changes in on a topic branch, so we can just ignore the Github actions for now.

---

In dyninst/testsuite, this line should fail:

I intentionally didn't run the test suite for the runs for the dashboard results because I knew it would break. We don't run the test suite in our actions because it was getting killed before it would finish even though it only ran for about 30 seconds. That's why we switched to using external-tests and then run the test suite on our machines nightly. We needed that anyway to cover arm and ppc.

Ah it's bc of this line here:

I don't see a Dockerfile 'move' instruction in their docs. Is 'RUN rm -rf /code\nCOPY . /code' a standard idiom? I'd like to tweak our current containers into thinner layers so that we don't have to carry forward the '/code' directory.

[jrmadsen]

@hainest if you want smaller layers, just do a multi-stage docker build. That way you can have as many layers you want into the first stage, remove all the unnecessary crud and the collapse all them in the final stage. There’s an example here from when I was at NERSC:

https://docs.nersc.gov/development/shifter/intel/#build-staging-example-using-nerscs-intel-docker-images

[vsoch]

@jrmadsen I helped with the docker setup, we already do a multistage build for the release container:

dyninst/docker/Dockerfile.release

Lines 33 to 35 in 231dd17

	COPY --from=builder /opt/dyninst-env /opt/dyninst-env
	COPY --from=builder /opt/spack /opt/spack
	COPY --from=builder /etc/profile.d/z10_spack_environment.sh /etc/profile.d/z10_spack_environment.sh

The others need to preserve the source code because we have to build it again, logically.

[jrmadsen]

@vsoch

The others need to preserve the source code because we have to build it again, logically.

I am not sure I see why you need to preserve the source code. In all the Dockerfiles, I see:

COPY . /code

It appears to me that after running build.sh, you can just rm -rf /code.

[vsoch]

@jrmadsen the Dockerfile.test is not pushed anywhere, it simply has source code added and rebuilt to run the tests. I'm not following your suggestion - what is the point of a multistage build here?

[jrmadsen]

@vsoch

I noticed this:

# Strip all the binaries
RUN find -L /opt/dyninst-env/* -type f -exec readlink -f '{}' \; | \
    xargs file -i | \
    grep 'charset=binary' | \
    grep 'x-executable\|x-archive\|x-sharedlib' | \
    awk -F: '{print $1}' | xargs strip -s

I think it might ultimately be a better idea to build this into CMake, e.g. add a DYNINST_STRIP option with a function like this in cmake:

function(DYNINST_STRIP_TARGET _TARGET)
    if(CMAKE_STRIP AND DYNINST_STRIP)
        add_custom_command(
            TARGET ${_TARGET}
            POST_BUILD
            COMMAND
                ${CMAKE_STRIP} -s ${ARGN} $<TARGET_FILE:${_TARGET}>
            WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
            COMMENT "Stripping ${_TARGET}...")
    endif()
endfunction()

[vsoch]

Gotcha, but what/how does this relate to multistage build?

[jrmadsen]

It doesn't, just something I noticed while looking through the Dockerfiles

[jrmadsen]

the Dockerfile.test is not pushed anywhere, it simply has source code added and rebuilt to run the tests. I'm not following your suggestion - what is the point of a multistage build here?

I brought up the multi-stage build bc @hainest said this:

I'd like to tweak our current containers into thinner layers so that we don't have to carry forward the '/code' directory

But the source code doesn't need to be retained in the container bc Dockerfile.test does a COPY . /code

[vsoch]

Gotcha. The reason we have it there is because the base container being used, ghcr.io/dyninst/dyninst-ubuntu-20.04:latest, is technically our development container - it comes with the build and the source code, so someone could quickly grab and run it to mess around without dependencies on the host. This isn't explicitly for the testing case here, but it works because it comes with all the dependencies, so it's trivial to replace the content in /code with what we want to test and then do it. The reason we don't use the release container (which is multistage) is because it removes the build dependencies too so we could not build the test code.

So if I understand correctly, what is required here is another container - a base that includes all dyninst dependencies but not the actual source code. You could either take the current https://github.com/dyninst/dyninst/blob/master/docker/Dockerfile and remove everything except for installing dyninst, or (I suspect Tim's preference) would be to have a more vanilla base (not relying on spack) that does the same. But either way, making Dockerfile.test a multistage build I do not think is the solution you seek.

[jrmadsen]

it comes with the build and the source code, so someone could quickly grab and run it to mess around without dependencies on the host. This isn't explicitly for the testing case here, but it works because it comes with all the dependencies, so it's trivial to replace the content in /code with what we want to test and then do it.

This seems like a weird approach to me. I understand creating a development container with the dependencies pre-installed but including the source and an existing build doesn't make a whole lot of sense to me because:

• Retaining the source code can create issues like the ones noted in Testing suite container bug when files are renamed #1272
• Retaining the build directory bloats the image
• If you reuse the build directory, you can end up with CMakeCache.txt artifacts that may go unnoticed
  • the perfect example being that if someone wants to "mess around with the dependencies on the host", you already have cached paths to these dependencies so, say installing a different boost version and re-building wouldn't actually do anything because CMakeCache.txt will (unless you override them) still use the boost libraries from the original build

[vsoch]

This seems like a weird approach to me. I understand creating a development container with the dependencies pre-installed but including the source and an existing build doesn't make a whole lot of sense to me because:

The expectation, as I noted above, is that it has the full source code if you want to work on it but don't have it locally.

Retaining the source code can create issues like the ones noted in #1272

Yes, and I'll again note this base container was not originally intended for testing, but was an easy/quick way to try it out.

Retaining the build directory bloats the image

See my first point.

If you reuse the build directory, you can end up with CMakeCache.txt artifacts that may go unnoticed

See my second point.

If these things are important to you, you are welcome to contribute change! Such as a new testing base...

[jrmadsen]

So if I understand correctly, what is required here is another container - a base that includes all dyninst dependencies but not the actual source code.

Actually, I think you should not include the source code or the build directory in the container. You can easily just do this when you will down the development container:

docker run -it --rm -v ${PWD}:/code ghcr.io/dyninst/dyninst-ubuntu-20.04:latest

and mount your code into the container.

[vsoch]

I'm signing off from this conversation, cheers.

[jrmadsen]

@vsoch If you prefer this approach, that is fine with me. I am just offering unsolicited alternatives bc the changes in this PR artificially passed testing bc of this approach.

[hainest]

I re-ran the pipeline with our updated container images, but it's still passing. I'm going to go ahead and merge this into the topic branch and deal with that later.

[jrmadsen]

@hainest I think that is bc you didn't rebase this branch (i.e. git pull upstream master)? In the log of Test Dyninst:

Step 1/9 : ARG dyninst_base=ghcr.io/dyninst/dyninst-ubuntu-20.04:latest
Step 2/9 : FROM ${dyninst_base}
latest: Pulling from dyninst/dyninst-ubuntu-20.04
... blah ...
 ---> 2d61d1825baa
Step 3/9 : COPY . /code
 ---> 1fba442a875b

Step 3 should be the RUN ... and Step 4 should be the COPY ...