Skip to content

remove redundant ARG, add build command to readme, build using nproc processes #4

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
Merged
merged 5 commits into from
Jul 18, 2024
Merged

remove redundant ARG, add build command to readme, build using nproc processes #4

merged 5 commits into from
Jul 18, 2024

Conversation

matter-it-does
Copy link
Contributor

@matter-it-does matter-it-does commented Jul 9, 2024 •
edited
Loading

  • nproc is used to use all cores on a machine not just 4 to speed up building.
  • Add an example to build the image because who remembers all the args and flags
  • remove redundant postgres_version arg

@matter-it-does matter-it-does changed the title fix ARG and apt postgres version fix ARG, apt postgres version, add build command to readme Jul 9, 2024
@matter-it-does
Copy link
Contributor Author

poke @rvianello for review

matter-it-does
matter-it-does commented Jul 9, 2024
View reviewed changes
Dockerfile
@@ -79,7 +80,6 @@ RUN initdb -D /opt/RDKit-build/pgdata \
&& pg_ctl -D /opt/RDKit-build/pgdata stop


ARG postgres_image_version=16.2
Copy link
Contributor Author

@matter-it-does matter-it-does Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's no need to set it again here since this VAR goes out of scope when the next FROM block starts.
And the top level definition takes effect

Copy link
Owner

Choose a reason for hiding this comment

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

good to know, thanks!

@matter-it-does matter-it-does mentioned this pull request Jul 9, 2024
Closed
@matter-it-does matter-it-does changed the title fix ARG, apt postgres version, add build command to readme fix ARG, apt postgres version, add build command to readme, build using nproc processes Jul 9, 2024
@rvianello
Copy link
Owner

Hi @matter-it-does, thanks a lot for this PR. I saw your comment on the other PR, but there is no description and I'm still unclear about what problem is solved by these changes.

In particular, can you help me understand why the installation of postgresql-server-dev-${postgres_version} required a fix, and why the modified version should work better?

@matter-it-does
Copy link
Contributor Author

@rvianello updated

@rvianello
Copy link
Owner

@matter-it-does thanks for updating the description. if you could briefly investigate the reason why you had to change that installation command, that would be great. I am honestly curious to understand why other people experienced errors and it always worked for me (and I actually seem to remember that I at some point was actually required to pin the package revision to get a version of the server-dev package that matched the postgres version from the parent docker image ..).

@matter-it-does
Copy link
Contributor Author

matter-it-does commented Jul 10, 2024 •
edited
Loading

I get Version 12.17* for postgresql-server-dev-12 was not found for both x86_64 and arm64
same for 12.16*...

@matter-it-does
Copy link
Contributor Author

anything else I can provide ?

@rvianello
Copy link
Owner

@matter-it-does sorry about the slow reply, I might have some time to run a couple of tests either later today or during the weekend.

@rvianello
Copy link
Owner

I performed a couple of test run and I think the command is not misbehaving, it's just that the deb packages for the older (<12.19 in this case) revisions of the server dev package are no longer available.. So my question now is also about why would one want to built the docker image for a postgres minor version that is not the most current one, and whether using a mismatching revision of the server dev package is really a good idea...

What about leaving the default behavior as it is, and maybe allow overriding it and dropping the pkg revision constraint based on a build argument?

@matter-it-does
Copy link
Contributor Author

matter-it-does commented Jul 12, 2024 •
edited
Loading

Ok got it. Thanks for the tests.
I don't control the DB version in my team. So I'm just reproducing it locally as is.
I'm fine either way.. I'll remove that postgres change from the PR and I think the rest can go. You can do a squash merge for clean commit history.

@matter-it-does matter-it-does changed the title fix ARG, apt postgres version, add build command to readme, build using nproc processes fix ARG, add build command to readme, build using nproc processes Jul 12, 2024
@matter-it-does matter-it-does changed the title fix ARG, add build command to readme, build using nproc processes remove redundant ARG, add build command to readme, build using nproc processes Jul 12, 2024
@matter-it-does
Copy link
Contributor Author

Are the packages present in apt archive by any chance ? apt-archive.postgresql.org

@rvianello
Copy link
Owner

I thought the same, but when I tried looking for more packages from apt-archive I still couldn't find them. I'm wondering if older revisions are regularly removed, or if they are removed every time a patch release is motivated by a known vulnerability.

@rvianello rvianello merged commit 2575b48 into rvianello:master Jul 18, 2024
@rvianello
Copy link
Owner

thanks a lot @matter-it-does for this PR and the discussion about the installallation of the server-dev package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvianello rvianello rvianello left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@matter-it-does @rvianello