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

add the docker specific arg primitive. #334

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add the docker specific arg primitive. #334

wants to merge 4 commits into from

Conversation

aavbsouza
Copy link
Contributor

… string for the shell and singularity container types

Pull Request Description

Add the the arg primitive that is docker specific. This primitive return a empty string for the bash and singularity container types.
Environment variables for these containers are already treated by the environment primitive.

Author Checklist

  • Updated documentation (pydocmd generate) if any docstrings have been modified
  • Passes all unit tests

Copy link
Collaborator

@samcmill samcmill left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

test/test_arg.py Outdated Show resolved Hide resolved
test/test_arg.py Outdated Show resolved Hide resolved
test/test_arg.py Outdated Show resolved Hide resolved
hpccm/primitives/arg.py Outdated Show resolved Hide resolved
hpccm/primitives/__init__.py Outdated Show resolved Hide resolved
"""String representation of the primitive"""
if self.__variables:

if hpccm.config.g_ctype == container_type.SINGULARITY or \
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: apptainer/singularity#1676

It is possible to get something like the Dockerfile ARG functionality in Singularity with this approach:
SINGULARITYENV_FOO=bar singularity build foo.sif Singularity.def

where Singularity.def contains

%post
    echo $FOO >> /bar

So I wonder if the "nodefault" case is applicable to Singularity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is possible to simulate that use case for Singularity. But since for singularity is not as clear the difference of ARG and ENV I do not know if it is better to treat the build time variables using the environment primitive

hpccm/primitives/arg.py Outdated Show resolved Hide resolved
@aavbsouza
Copy link
Contributor Author

Hello @samcmill. I have changed the PR to also work with singularity and bash containers. For the Singularity case I have used your suggestion for the bash syntax to define a variable.

Copy link
Collaborator

@samcmill samcmill left a comment

Choose a reason for hiding this comment

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

Sorry, lost track of this over the holidays. Looking good.

string += "%post" + "\n"
for count, (key, val) in enumerate(sorted(variables.items())):
eol = "" if count == num_vars - 1 else "\n"
string += ' {0}=${{{0}:-"{1}"}}'.format(key, val) + eol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do the bash and Singularity args need to be exported? (I'm not sure)

@aavbsouza
Copy link
Contributor Author

Hello @samcmill any toughs on the current state of the PR ?

@samcmill
Copy link
Collaborator

@aavbsouza did you see #334 (comment)? Otherwise, this looks good to me.

@aavbsouza
Copy link
Contributor Author

Hello @samcmill sorry for the delay. I will update the pull request. thanks

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.

2 participants