tjwp commented on code in PR #2191: URL: https://github.com/apache/avro/pull/2191#discussion_r1173925828
########## lang/ruby/Gemfile: ########## @@ -16,8 +16,14 @@ source 'https://rubygems.org' -VERSION = File.open('../../share/VERSION.txt').read.sub('-SNAPSHOT', '.pre1').chomp -File.write("lib/avro/VERSION.txt", VERSION) +# Add the VERSION.txt if it exists, or use a fake one for preinstalling gems +version_path = '../../share/VERSION.txt' +if File.exist?(version_path) + VERSION = File.open(version_path).read.sub('-SNAPSHOT', '.pre1').chomp + File.write("lib/avro/VERSION.txt", VERSION) +else + File.write("/tmp/lib/avro/VERSION.txt", "0.0.1-fake") +end Review Comment: Since the `VERSION.txt` file is kept in git maybe it is better to keep the original code here and ensure that `share/VERSION.txt` is copied like the Gemfile, gemspec, and Manifest? ########## share/docker/Dockerfile: ########## @@ -179,6 +179,11 @@ RUN python3 -m pip install --upgrade pip setuptools wheel \ # Install Ruby RUN apt-get -qqy install ruby-full \ && apt-get -qqy clean +COPY lang/ruby/* /tmp/ +RUN gem install bundler -v 1.17.3 --no-document && \ Review Comment: bundler 1.17.3 is pretty old now. I tested your changes without specifying the version and everything still works. ```suggestion RUN gem install bundler --no-document && \ ``` ########## build.sh: ########## @@ -300,8 +300,9 @@ do echo "RUN getent group $GROUP_ID || groupadd -g $GROUP_ID $USER_NAME" echo "RUN getent passwd $USER_ID || useradd -g $GROUP_ID -u $USER_ID -k /root -m $USER_NAME" } > Dockerfile + # Include the ruby gemspec for preinstallation. # shellcheck disable=SC2086 - tar -cf- lang/ruby/Gemfile Dockerfile | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" - + tar -cf- Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" - Review Comment: Suggest adding `share/VERSION.txt` here too: ```suggestion tar -cf- Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest share/VERSION.txt | docker build $DOCKER_BUILD_XTRA_ARGS -t "$DOCKER_IMAGE_NAME" - ``` ########## share/docker/Dockerfile: ########## @@ -179,6 +179,11 @@ RUN python3 -m pip install --upgrade pip setuptools wheel \ # Install Ruby RUN apt-get -qqy install ruby-full \ && apt-get -qqy clean +COPY lang/ruby/* /tmp/ +RUN gem install bundler -v 1.17.3 --no-document && \ + apt-get install -qqy libyaml-dev && \ + mkdir -p /tmp/lib/avro/ && \ + bundle install --gemfile=/tmp/Gemfile Review Comment: If the `share/VERSION.txt` is copied then here is a suggestion to replicate the expected directory structure and keep the `Gemfile` simpler (removal of the bundler version also included): ```suggestion RUN mkdir -p /tmp/lang/ruby/lib/avro && mkdir -p /tmp/share COPY lang/ruby/* /tmp/lang/ruby/ COPY share/VERSION.txt /tmp/share/ RUN gem install bundler --no-document && \ apt-get install -qqy libyaml-dev && \ cd /tmp/lang/ruby && bundle install ``` ########## build.sh: ########## @@ -336,7 +337,7 @@ do ;; docker-test) - tar -cf- share/docker/Dockerfile lang/ruby/Gemfile | + tar -cf- share/docker/Dockerfile lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest | Review Comment: Suggest adding `share/VERSION.txt` here too. To keep the list of extra files in sync between `docker` and `docker-test` targets it may be worth extracting a variable like: ``` EXTRA_DOCKER_CONTEXT="lang/ruby/Gemfile lang/ruby/avro.gemspec lang/ruby/Manifest share/VERSION.txt" ``` And then reference that here and above in the `docker` target: ```suggestion tar -cf- share/docker/Dockerfile $EXTRA_DOCKER_CONTEXT | ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
