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.

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

rework yum-requirements #1019

Closed
wants to merge 3 commits into from
Closed

rework yum-requirements #1019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0993737
rework yum-requirements to make it install the requirements in a cont…
mariusvniekerk Feb 16, 2019
0cf7b8e
Seems we can do this just inside
mariusvniekerk Feb 17, 2019
b51c8cd
remove deprecated test
mariusvniekerk Feb 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 0 additions & 46 deletions conda_smithy/configure_feedstock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,11 +708,6 @@ def _circle_specific_setup(jinja_env, forge_config, forge_dir, platform):
"""
)

if platform == "linux":
yum_build_setup = generate_yum_requirements(forge_dir)
if yum_build_setup:
forge_config["yum_build_setup"] = yum_build_setup

forge_config["build_setup"] = build_setup

if platform == "linux":
Expand Down Expand Up @@ -744,42 +739,6 @@ def _circle_specific_setup(jinja_env, forge_config, forge_dir, platform):
set_exe_file(target_fname, True)


def generate_yum_requirements(forge_dir):
# If there is a "yum_requirements.txt" file in the recipe, we honour it.
yum_requirements_fpath = os.path.join(
forge_dir, "recipe", "yum_requirements.txt"
)
yum_build_setup = ''
if os.path.exists(yum_requirements_fpath):
with open(yum_requirements_fpath) as fh:
requirements = [
line.strip()
for line in fh
if line.strip() and not line.strip().startswith("#")
]
if not requirements:
raise ValueError(
"No yum requirements enabled in the "
"yum_requirements.txt, please remove the file "
"or add some."
)
yum_build_setup = textwrap.dedent(
"""\

# Install the yum requirements defined canonically in the
# "recipe/yum_requirements.txt" file. After updating that file,
# run "conda smithy rerender" and this line will be updated
# automatically.
/usr/bin/sudo -n yum install -y {}


""".format(
" ".join(requirements)
)
)
return yum_build_setup


def _get_platforms_of_provider(provider, forge_config):
platforms = []
keep_noarchs = []
Expand Down Expand Up @@ -1014,11 +973,6 @@ def _azure_specific_setup(jinja_env, forge_config, forge_dir, platform):
# Explicitly add in a newline character to ensure that jinja templating doesn't do something stupid
build_setup = "run_conda_forge_build_setup\n"

if platform == "linux":
yum_build_setup = generate_yum_requirements(forge_dir)
if yum_build_setup:
forge_config['yum_build_setup'] = yum_build_setup

forge_config["build_setup"] = build_setup

forge_config["docker"]["interactive"] = False
Expand Down
2 changes: 1 addition & 1 deletion conda_smithy/templates/azure-pipelines-linux.yml.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
# configure qemu binfmt-misc running. This allows us to run docker containers
# embedded qemu-static
- script: |
docker run --rm --privileged multiarch/qemu-user-static:register
docker run --rm --privileged multiarch/qemu-user-static:register --reset --credential yes
ls /proc/sys/fs/binfmt_misc/
condition: not(startsWith(variables['CONFIG'], 'linux_64'))
displayName: Configure binfmt_misc
Expand Down
11 changes: 8 additions & 3 deletions conda_smithy/templates/build_steps.sh.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export CONFIG_FILE="${CI_SUPPORT}/${CONFIG}.yaml"
cat >~/.condarc <<CONDARC

conda-build:
root-dir: /home/conda/feedstock_root/build_artifacts
root-dir: /home/conda/feedstock_root/build_artifacts

CONDARC

Expand All @@ -26,8 +26,13 @@ setup_conda_rc "${FEEDSTOCK_ROOT}" "${RECIPE_ROOT}" "${CONFIG_FILE}"

{% if build_setup -%}
{{ build_setup }}{% endif -%}
{% if yum_build_setup is defined -%}
{{ yum_build_setup }}{% endif -%}

# If we have yum requirements build a new specialized image that we will use to run our build in that includes
# those dependencies.
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this comment or remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Actually are we able to just revert all of the changes to this file now?

Copy link
Member Author

@mariusvniekerk mariusvniekerk Feb 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we could, though this does add the advantage that we dont need a rerender if we've changed yum_requirements, and it simplifies configure_feedstock.py

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is roughly how I proposed yum_requirements.txt originally ( #135 ), but Phil made a compelling argument that they should be listed explicitly ( #134 ). Ultimately I don't feel strongly either way, but it seems the crux of the needed change is quite simple; so, it would be nice if this PR reflected that. Orthogonal changes are better left for other PRs IMHO.

if [ -f ${RECIPE_ROOT}/yum_requirements.txt ]; then
sudo yum -y install $(cat ${RECIPE_ROOT}/yum_requirements.txt)
fi


# make the build number clobber
make_build_number "${FEEDSTOCK_ROOT}" "${RECIPE_ROOT}" "${CONFIG_FILE}"
Expand Down
23 changes: 14 additions & 9 deletions conda_smithy/templates/run_docker_build.sh.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ mkdir -p "$ARTIFACTS"
DONE_CANARY="$ARTIFACTS/conda-forge-build-done-${CONFIG}"
rm -f "$DONE_CANARY"

# Set up the docker image that we are going to use.
{{ docker.executable }} pull $DOCKER_IMAGE

{%- if docker.interactive is defined %}
{%- if docker.interactive %}
# Enable running in interactive mode attached to a tty
Expand All @@ -53,15 +56,17 @@ DOCKER_RUN_ARGS=" -it "

export UPLOAD_PACKAGES="${UPLOAD_PACKAGES:-True}"
{{ docker.executable }} run ${DOCKER_RUN_ARGS} \
-v "${RECIPE_ROOT}":/home/conda/recipe_root:ro,z \
-v "${FEEDSTOCK_ROOT}":/home/conda/feedstock_root:rw,z \
-e CONFIG \
-e BINSTAR_TOKEN \
-e HOST_USER_ID \
-e UPLOAD_PACKAGES \
$DOCKER_IMAGE \
{{ docker.command }} \
/home/conda/feedstock_root/${PROVIDER_DIR}/build_steps.sh
-v "${RECIPE_ROOT}":/home/conda/recipe_root:ro,z \
-v "${FEEDSTOCK_ROOT}":/home/conda/feedstock_root:rw,z \
-e CONFIG \
-e BINSTAR_TOKEN \
-e HOST_USER_ID \
-e UPLOAD_PACKAGES \
$DOCKER_IMAGE \
{{ docker.command }} \
/home/conda/feedstock_root/${PROVIDER_DIR}/build_steps.sh
Copy link
Member

Choose a reason for hiding this comment

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

Also are we able to revert the changes here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could, but this is just whitespace

Copy link
Member

Choose a reason for hiding this comment

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

Exactly




# verify that the end of the script was reached
test -f "$DONE_CANARY"
25 changes: 25 additions & 0 deletions news/yum-rework.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
**Added:**

* <news item>

**Changed:**

* Changed how yum_requirements.txt is used in the docker build. It now installs the yum_requirements
prior to doing the primary conda build without needing to use sudo.

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* <news item>

**Security:**

* <news item>

13 changes: 0 additions & 13 deletions tests/test_configure_feedstock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,19 +149,6 @@ def test_circle_with_yum_reqs(py_recipe, jinja_env):
)


def test_circle_with_empty_yum_reqs_raises(py_recipe, jinja_env):
with open(
os.path.join(py_recipe.recipe, "recipe", "yum_requirements.txt"), "w"
) as f:
f.write("# effectively empty")
with pytest.raises(ValueError):
cnfgr_fdstk.render_circle(
jinja_env=jinja_env,
forge_config=py_recipe.config,
forge_dir=py_recipe.recipe,
)


def test_circle_osx(py_recipe, jinja_env):
forge_dir = py_recipe.recipe
travis_yml_file = os.path.join(forge_dir, ".travis.yml")
Expand Down