Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Source Distribution] Add script to regenerate configure file #7582

Merged
merged 1 commit into from
Jan 18, 2023
Merged

[Source Distribution] Add script to regenerate configure file #7582

merged 1 commit into from
Jan 18, 2023

Conversation

stealthycoin
Copy link
Contributor

No description provided.

@stealthycoin stealthycoin requested a review from kyleknap January 13, 2023 00:11
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023 •
edited
Loading

Codecov Report

Merging #7582 (eb44ba1) into source-distribution (2e0f1d4) will not change coverage.
The diff coverage is n/a.

❗ Current head eb44ba1 differs from pull request most recent head abda8d4. Consider uploading reports for the commit abda8d4 to get more accurate results

@@                 Coverage Diff                  @@
##           source-distribution    #7582   +/-   ##
====================================================
  Coverage                93.01%   93.01%           
====================================================
  Files                      354      354           
  Lines                    37448    37448           
  Branches                  5433     5433           
====================================================
  Hits                     34833    34833           
  Misses                    1960     1960           
  Partials                   655      655

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kyleknap
kyleknap reviewed Jan 13, 2023
View reviewed changes
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. Just had a couple of comments on the dockerfile

scripts/regenerate-configure/Dockerfile Outdated
WORKDIR /build
COPY ../ /build
RUN yum -y update
RUN yum -y install curl xz gzip tar automake make
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to pin automake as well by building/installing from source as I believe the m4 macros we use for Python live in the automake package and that impacts the end configure file generated

scripts/regenerate-configure/Dockerfile Outdated
@@ -0,0 +1,17 @@
FROM amazonlinux
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the Amazon Linux image from the ECR public repository so we don't necessarily need DockerHub creds to pull (in case of throttles) and pin this to major version 2 tag so that there is some stability in the base image.

kyleknap
kyleknap reviewed Jan 17, 2023
View reviewed changes
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks fine. Just had a comment about our configure help output

configure Outdated
@@ -1239,6 +1242,11 @@ Installation directories:
Optional Packages:
--with-PACKAGE[=ARG] use PACKAGE [ARG=yes]
--without-PACKAGE do not use PACKAGE (same as --with-PACKAGE=no)
--with-python-sys-prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are using a more recent version of automake. We probably want to pull in this m4 macro to update the help output that we had before: https://github.com/aws/aws-cli/pull/7565/files#diff-a8b976bc5f63366bb7d38a6e3277c5134a59535dd55169f8427622e970176b70R2

@stealthycoin
Add script to regenerate configure file
abda8d4 
Script to regenerate configure file uses pinned versions of automake
and autotools inside a docker image to ensure consistent configure
output.
@stealthycoin stealthycoin mentioned this pull request Jan 17, 2023
Closed
5 tasks
kyleknap
kyleknap approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good 🚢

@stealthycoin stealthycoin merged commit b2112f9 into aws:source-distribution Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyleknap kyleknap kyleknap approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stealthycoin @codecov-commenter @kyleknap