Replace os.getcwd() usage in fre/make container build scripts (#945) - #951
Replace os.getcwd() usage in fre/make container build scripts (#945)#951aabdulai116 wants to merge 1 commit into
Conversation
No. I'm not a bot. This was my first ever open source pull request. I'm a beginner, and I was honestly excited that it was going to a NOAA project because I built a forest carbon calculator myself. |
|
we've had an unprecedented number of bot contributions today, on exactly issues like the one you've chosen, so i am sorry, but you need more than a simple "no I am not a bot". ... that's what a bot would say |
Thanks for pointing that. I really appreciate it and I would also work on changing my profile to prevent people from thinking that I am a bot. This is a project that I built and I have the repo on my page (https://github.com/aabdulai116/FOREST-CARBON-CALCULATOR-arbocalc-) |
|
@aabdulai116 Thank you for your contribution. I am one of the NOAA-GFDL admins on GitHub. This is code that I personally wrote, and I am interested in reviewing your contribution, so I've reopened your PR. Are you interested in using fre-cli? Do you have experience with creating dockerfiles, are you more of a python person, or both? |
There was a problem hiding this comment.
Pull request overview
This PR removes os.getcwd() usage from the fre/make container build script generation paths, aligning with the broader effort in #942/#945 to avoid brittle assumptions about the caller’s working directory while preserving existing behavior.
Changes:
- Compute and reuse an absolute path for
createContainer.shincreateBuildScript()instead of concatenatingos.getcwd(). - Remove
currDir = os.getcwd()fromcreate_docker_script.pyand derive log locations fromdockerBuild.userScriptPath.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
fre/make/gfdlfremake/buildDocker.py |
Replaces relative/open+chmod path handling with a single os.path.abspath()-computed script path. |
fre/make/create_docker_script.py |
Removes os.getcwd() and updates INFO logs to use the build script directory derived from userScriptPath. |
| fre_logger.info("tmpDir created in " + os.path.join(buildDir, "tmp")) | ||
| fre_logger.info("Dockerfile created in " + buildDir) |
thomas-robinson
left a comment
There was a problem hiding this comment.
Thanks for this code. I believe it needs an update to find where the dockerfile is opened and not the docker build script to be sure the tmp directory is in the right place. The build script and dockerfile should be in the same folder, and maybe there should be a check to make sure the paths are all correct. Please update to use the dockerfile folder path and check to make sure the dockerfile and build script are in the same folder. Also, copilot had a suggestion for updating the logged message. Please address that too. Let me know if you need more details.
|
|
||
| # create build script for container | ||
| dockerBuild.createBuildScript(platform, skip_format_transfer = no_format_transfer) | ||
| buildDir = os.path.dirname(dockerBuild.userScriptPath) |
There was a problem hiding this comment.
I don't think this is fully safe. We need the path to the dockerfile here, not the script that builds the container, although they should be the same. The contents of this tmp directory are referenced in the dockerfile. The files are generated on the system by fre-cli and then copied in to the container. I think to be safe here, you need to see where you are when the dockerfile is opened, not the script.
Describe your changes
Removes both os.getcwd() calls in fre/make, following the reasoning in #942:
fre/make/gfdlfremake/buildDocker.py (createBuildScript): the absolute path of
createContainer.sh is now computed once with os.path.abspath() and reused for
open(), os.chmod(), and self.userScriptPath, instead of concatenating
os.getcwd() + "/createContainer.sh".
fre/make/create_docker_script.py: currDir = os.getcwd() is removed; the log
messages now derive the location from dockerBuild.userScriptPath via
os.path.dirname(), so they report where files were actually written.
No behavior change intended: the build script is still written to the caller's
working directory, as before.
Note: there are additional os.getcwd() calls under fre/make/tests/. I left those
out to keep this PR scoped to the two lines linked in #945 — happy to open a
follow-up PR covering the tests if that's wanted.
This is my first contribution to fre-cli , and any feedback welcome.
Issue ticket number and link (if applicable)
Fixes #945
Checklist before requesting a review
Verified both files compile with python -m py_compile; relying on CI for the full test suite.