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.

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

Provide PDO_SOURCE_ROOT default for docker to allow zero-config build #478

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

g2flyer
Copy link
Contributor

@g2flyer g2flyer commented Mar 15, 2024

Minor change to allow docker builds out-of-the-box without any configuration/environment variable definition. This default for PDO_SOURCE_ROOT also hides an error for undefined PDO_SOURCE_ROOT which is very unintuitive to figure out if you haven't run into this issue beforehand.

@g2flyer g2flyer force-pushed the msteiner.zero-config-docker branch 2 times, most recently from 815e891 to 5f2dacc Compare March 15, 2024 01:44
@g2flyer g2flyer requested a review from cmickeyb March 15, 2024 02:20
@cmickeyb cmickeyb self-assigned this Mar 15, 2024
docker/Makefile Outdated
@@ -14,6 +14,9 @@
# limitations under the License.
# ------------------------------------------------------------------------------

# PDO_SOURCE_ROOT
PDO_SOURCE_ROOT ?= $(shell readlink -f ..)
Copy link
Member

Choose a reason for hiding this comment

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

This assumes make is running within the docker folder, which is not the case if you run with the -f option outside of it -- this does not happen to us usually, but it's a possibility.

Suggested change
PDO_SOURCE_ROOT ?= $(shell readlink -f ..)
mkfile_path := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
PDO_SOURCE_ROOT ?= $(realpath $(mkfile_path)../)

The proposed change makes the computation of the parent folder dependent on the makefile location itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooops, given that i usually use make -C docker .. i should have known better. Thanks for catching. Pushed corresponding update merging in your changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

by the way... that $(lastword $(MAKEFILE_LIST)) does not work if there is a space in the path name. so i'm not sure that there is a "perfect" way to do this.

Copy link
Contributor Author

@g2flyer g2flyer Mar 15, 2024

Choose a reason for hiding this comment

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

by the way... that (lastword(MAKEFILE_LIST)) does not work if there is a space in the path name. so i'm not sure that there is a "perfect" way to do this.

good point. But somebody on unix/linux having spaces in paths is calling for trouble. Stackoverflow also argues that make does not support filenames with whitespace in them.. $(CURDIR) would work for the -C case but alas would not work for the -f case. I would tend to leave it as-is? Definitely better than my original version and are we sure everything else works when there are spaces in paths? never mind: $(CURDIR) just would have been a more elegant but equivalent form to my original version..

Copy link
Contributor

Choose a reason for hiding this comment

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

one small request... we define DOCKER_DIR farther down in the Makefile. Can you move that up and set it instead of mkkfile_path (to be clear... it was ALREADY in the makefile, farther down, with the comment about why it was not being set with MAKEFILE_LIST). That being said... its set using MAKEFILE_LIST in the pdo-contracts repo.

@g2flyer g2flyer force-pushed the msteiner.zero-config-docker branch from 5f2dacc to a8009f6 Compare March 15, 2024 19:07
@g2flyer g2flyer requested a review from bvavala March 15, 2024 19:09
@g2flyer g2flyer force-pushed the msteiner.zero-config-docker branch from a8009f6 to de726ed Compare March 18, 2024 20:13
@g2flyer g2flyer force-pushed the msteiner.zero-config-docker branch from de726ed to 83cc11c Compare March 18, 2024 20:38
Copy link
Contributor

@cmickeyb cmickeyb left a comment

Choose a reason for hiding this comment

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

this looks great!

@cmickeyb cmickeyb merged commit da18b88 into hyperledger-labs:main Mar 18, 2024
4 checks passed
@g2flyer g2flyer deleted the msteiner.zero-config-docker branch March 18, 2024 20:58
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.

3 participants