Skip to content

Cleanup Dockerfiles by removing unused asn1c instructions and comments#48

Merged
dmccoystephenson merged 7 commits intodevelopfrom
docker/cleanup-dockerfiles
Sep 14, 2025
Merged

Cleanup Dockerfiles by removing unused asn1c instructions and comments#48
dmccoystephenson merged 7 commits intodevelopfrom
docker/cleanup-dockerfiles

Conversation

@dmccoystephenson
Copy link
Copy Markdown

@dmccoystephenson dmccoystephenson commented Sep 10, 2025

Problem

Previously, the doIt.sh script was updated to extract already-compiled header/implementation files rather than compiling them using asn1c. This decoupled the compilation process from the Docker build process, and the relevant asn1c installation instructions were commented out in the Dockerfiles. These commented-out instructions remain, cluttering the files.

Solution

Removed all commented-out commands and contextual comments related to asn1c installation from all Dockerfiles to improve readability and maintainability. No active commands, configurations, or environment settings were changed.

Testing

  • Verified that docker build completes successfully for each Dockerfile.
  • No functional changes were made, so no re-verification of image functionality is required.

Other Changes

  • A dependency was added to the ci.yml to fix a CI check failure.
  • The asn1c installation steps were removed from the ci.yml file since they are no longer required for the doIt.sh script.

@dmccoystephenson dmccoystephenson changed the title docker: remove unnecessary asn1c build steps from Dockerfiles Cleanup Dockerfiles by removing unused asn1c instructions and comments Sep 10, 2025
Comment thread .github/workflows/ci.yml
apt update
apt-get -y install sudo wget curl gnupg lsb-release gcovr unzip gcc-multilib libasan*
sudo apt-get -y install software-properties-common
sudo apt-get -y install libcurl4-openssl-dev
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This was necessary to get the CI checks to pass.

@dmccoystephenson dmccoystephenson marked this pull request as ready for review September 10, 2025 19:32
@pmonington
Copy link
Copy Markdown

Looks good to me!

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml Outdated
cd ./asn1c && aclocal && test -f configure || autoreconf -iv && ./configure && make && make install
working-directory: ${{ env.working-directory }}

- name: Generate ASN.1 API.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question(blocking): if we have moved away from the doIt.sh script, should this action be updated to use the generate-files.sh script instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doIt.sh script is now used to extract header & implementation files which are compiled separately by generate-files.sh. The former is what is required here. I've renamed this action to make it clear that we're extracting files rather than generating them.

Comment thread .github/workflows/ci.yml
- name: Extract implementation and header files
run: |
export LD_LIBRARY_PATH=/usr/local/lib
git clone https://github.com/usdot-jpo-ode/scms-asn1.git
Copy link
Copy Markdown
Author

@dmccoystephenson dmccoystephenson Sep 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: SCMS files are now tracked directly in the asn1_codec repo so this clone command was unnecessary.

Comment thread .github/workflows/ci.yml
- name: Install pugixml
run: |
git clone https://github.com/vlm/asn1c.git
git clone https://github.com/zeux/pugixml.git
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): it's not related to the changes in this PR, but do you know why we're explicitly cloning here and not using git submodule?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if there is a particular reasoning that the original author of ci.yml had, but this does introduce the potential for CI checks to use different versions of git submodule code than the actual repository so it may be a good idea to look into using the existing git submodules present when checking out the asn1_codec files.

Copy link
Copy Markdown

@payneBrandon payneBrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@dmccoystephenson dmccoystephenson merged commit 43bc2f9 into develop Sep 14, 2025
3 checks passed
@drewjj drewjj deleted the docker/cleanup-dockerfiles branch November 5, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants