fix: success check does not consider pkg_dir #836 #838

jpfeuffer · 2023-02-24T11:16:13Z

completely untested but it is an obvious bug.
If you want to change the package dirs earlier in the code somewhere, please suggest changes

jpfeuffer · 2023-02-26T17:02:02Z

@bgruening @daler Ready to review. Somehow I cannot request it from the normal UI.

daler · 2023-02-27T17:56:32Z

@jpfeuffer just to clarify, is OpenMS is using bioconda-utils for internal builds?

jpfeuffer · 2023-02-27T18:03:16Z

Hey @daler
yes, correct. We want to ensure the package builds on bioconda in the moment we trigger a release. And that nightly builds in-between are compatible with the rest of bioconda. If that is possible with pure conda build without too much hassle we can also use that.

daler · 2023-02-27T18:11:09Z

Ah, ok. Can you point to where (and how) you are calling it then? I bet if it's not exactly how we call it in bioconda-recipes then that might introduce behavior that we don't otherwise test for.

jpfeuffer · 2023-02-27T18:19:31Z

Sure! Yes, you might not be using the --docker-specific --pkg_dir option to specify a directory to be mounted into the docker container. We use it, so we can reuse already-built packages for the next run and generally have access to the finished packages after the build. I did not check if we can change the upload behavior otherwise that would have been an alternative.

# output must be a valid conda "channel" (otherwise I think I encountered problems)
mkdir -p output/noarch
touch output/noarch/repodata.json
conda activate bioconda
mamba update -y bioconda-utils
conda index output

git clone https://github.com/bioconda/bioconda-recipes && cd bioconda-recipes # we usually use our own fork with ntly changes
bioconda-utils build --docker --pkg_dir $WORKSPACE/output --packages openms-meta # feel free to choose any pkg
bioconda-utils build --docker --pkg_dir $WORKSPACE/output --packages pyopenms

Note: you cannot run them together because of #751
I think it also needed to be an absolute path (therefore $WORKSPACE)

daler · 2023-02-27T22:05:50Z

So the default behavior is for --pkg_dir to not be set, which in turn defaults to the identified conda-build directory on the host, which is where the built recipes will be copied to and mounted read-only into the container. So the default behavior does, I think, what you are aiming to do.

Does it work if you remove the --pkg_dir arg?

jpfeuffer · 2023-02-27T22:12:29Z

Yes, I think it works without --pkg_dir but somehow we ended up using a subfolder in the workspace (persistency, permission reasons I guess). We might be able to reevaluate the thing but I think it would be great if the build also works with --pkg_dir.

daler · 2023-02-27T22:19:40Z

I think it would be ok to merge as-is since we don't use --pkg_dir anywhere in production, but merging without tests always makes me nervous. And there are currently bigger issues to resolve that affect the entire channel (and very limited time to do so).

Can you write a test for this in the test dir, that fails in master branch but passes with your addition?

test/test_utils.py

jpfeuffer · 2023-02-28T18:01:30Z

@daler Ready. I am 90% sure it will fail on master.

jpfeuffer · 2023-03-03T14:45:27Z

@daler Do you need anything else?

timosachsenberg · 2023-03-20T13:31:45Z

Hi, has there been any progress with this PR?

johanneskoester · 2023-03-24T22:13:59Z

bioconda_utils/build.py

+                    platform = os.environ.get('OSTYPE', sys.platform)
+                    if platform.startswith("darwin"):
+                        platform = 'osx'
+                    elif platform == "linux-gnu":
+                        platform = "linux"


The platform can be inferred from RepoData().native_platform().

Also, make sure that you indent only with 4 spaces.

poshul · 2023-04-01T09:14:19Z

FAILED test/test_utils.py::test_single_build_pkg_dir - AssertionError: bug: ensure to load config before instantiating RepoData. ERROR test/test_utils.py::test_single_build_only[with docker] - AssertionError: bug: ensure to load config before instantiating RepoData.

jpfeuffer · 2023-04-07T17:10:51Z

Sorry, I don't know what that means. I merely added the review suggestion. I guess this error is also why the suggested code was not used in the place where I copied it from 🤷.
I guess we have to wait for further suggestions..

timosachsenberg · 2023-04-14T08:08:54Z

any update here?

poshul · 2023-05-15T09:18:53Z

@johanneskoester @daler any updates on this?

poshul · 2023-10-25T09:18:03Z

@johanneskoester @daler any updates?

timosachsenberg · 2024-11-19T14:49:03Z

any news?

bioconda_utils/build.py

coderabbitai · 2024-11-19T15:16:14Z

Walkthrough

The changes introduced in the pull request primarily affect two files: bioconda_utils/build.py and test/test_utils.py. In bioconda_utils/build.py, modifications include the addition of import statements for sys and time, which are likely intended for enhanced functionality or logging purposes. The build function has been updated to improve the handling of package paths when using a Docker builder. Specifically, it now retrieves the native platform and adjusts the pkg_paths based on the Docker builder's package directory if docker_builder.pkg_dir is not None. This aims to ensure accurate resolution of package paths during the build process. Additionally, error handling has been refined to provide better logging for package path issues post-build, although the overall control flow remains unchanged.

In test/test_utils.py, a new test function, test_single_build_pkg_dir, has been added to evaluate the build process using a specified package directory within a Docker context. Existing Docker-related tests have been annotated to skip on macOS platforms. These changes enhance the test suite's coverage without altering the logic of existing tests.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)

bioconda_utils/build.py (2)
8-11: Remove unused imports

The imports sys and time are not used in the code.

Apply this diff to remove the unused imports:
import logging
import os
-import sys
-import time
🧰 Tools

🪛 Ruff

10-10: sys imported but unused

Remove unused import: sys

(F401)

11-11: time imported but unused

Remove unused import: time

(F401)

145-150: LGTM! The fix correctly handles pkg_dir in Docker builds

The implementation properly adjusts package paths when using a Docker builder with a custom package directory, fixing the success check issue.

Consider these improvements:

Add type hints for pkg_paths parameter

Add error handling for platform conversion
-def build(recipe: str, pkg_paths: List[str] = None,
+def build(
+    recipe: str,
+    pkg_paths: Optional[List[str]] = None,
     ...
 ):
     ...
     if docker_builder.pkg_dir is not None:
         try:
             platform = utils.RepoData.native_platform()
             subfolder = utils.RepoData.platform2subdir(platform)
             conda_build_config = utils.load_conda_build_config(platform=subfolder)
             pkg_paths = [p.replace(conda_build_config.output_folder, docker_builder.pkg_dir) for p in pkg_paths]
+        except Exception as e:
+            logger.error("Failed to adjust package paths: %s", str(e))
+            return BuildResult(False, None)
test/test_utils.py (3)
255-277: LGTM! The test effectively validates pkg_dir handling.

The test is well-structured and directly addresses the PR objective of fixing pkg_dir handling in the build process. It properly sets up a docker builder with a specific pkg_dir and verifies the build result.

Consider adding cleanup of the output directory after the test:
 def test_single_build_pkg_dir(recipes_fixture):
     """
     Builds the "one" recipe with pkg_dir.
     """
+    output_dir = os.getcwd() + "/output"
     logger.error("Making recipe builder")
     docker_builder = docker_utils.RecipeBuilder(
         use_host_conda_bld=True,
-        pkg_dir=os.getcwd() + "/output",
+        pkg_dir=output_dir,
         docker_base_image=DOCKER_BASE_IMAGE)
     mulled_test = False
     logger.error("DONE")
     logger.error("Fixture: Building 'one' within docker with pkg_dir")
     res = build.build(
         recipe=recipes_fixture.recipe_dirs['one'],
         pkg_paths=recipes_fixture.pkgs['one'],
         docker_builder=docker_builder,
         mulled_test=mulled_test,
     )
     logger.error("Fixture: Building 'one' within docker and pkg_dir -- DONE")
     assert res.success
+    # Cleanup
+    if os.path.exists(output_dir):
+        shutil.rmtree(output_dir)
261-262: Improve error logging message.

The log message could be more descriptive about what's being created.
-    logger.error("Making recipe builder")
+    logger.error("Creating docker recipe builder with pkg_dir")
268-269: Enhance build logging message.

The log message could include the pkg_dir path for better debugging.
-    logger.error("Fixture: Building 'one' within docker with pkg_dir")
+    logger.error("Fixture: Building 'one' within docker with pkg_dir: %s", docker_builder.pkg_dir)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ce37553 and 386b376.

📒 Files selected for processing (2)

bioconda_utils/build.py (2 hunks)
test/test_utils.py (1 hunks)

🧰 Additional context used

🪛 Ruff

bioconda_utils/build.py

10-10: sys imported but unused

Remove unused import: sys

(F401)

11-11: time imported but unused

Remove unused import: time

(F401)

bgruening

Thanks everyone!

🤖 I have created a release \*beep\* \*boop\* --- ## [3.5.0](https://www.github.com/bioconda/bioconda-utils/compare/v3.4.1...v3.5.0) (2024-11-29) ### Features * add rust to should_use_compilers lint ([#1018](https://www.github.com/bioconda/bioconda-utils/issues/1018)) ([0dacce9](https://www.github.com/bioconda/bioconda-utils/commit/0dacce91865e649061b7d18008c34789287c65fa)) ### Bug Fixes * success check does not consider pkg_dir [#836](https://www.github.com/bioconda/bioconda-utils/issues/836) ([#838](https://www.github.com/bioconda/bioconda-utils/issues/838)) ([6240d31](https://www.github.com/bioconda/bioconda-utils/commit/6240d31e706b91b4b217ab10305fb84aabd8a147)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fix bioconda#836

fb369fc

jpfeuffer changed the title ~~fix #836~~ fix: success check does not consider pkg_dir #836 Feb 24, 2023

jpfeuffer added 2 commits February 24, 2023 14:17

forgot sys import

f53f7d0

forgot module

a61b5b7

Merge branch 'master' into jpfeuffer-patch-1

05cfe98

jpfeuffer added 3 commits February 27, 2023 23:32

try to add some tests

39e92b7

forgot comma

0e66d3b

mark test for a test category

8369757

jpfeuffer commented Feb 28, 2023

View reviewed changes

test/test_utils.py Outdated Show resolved Hide resolved

jpfeuffer commented Feb 28, 2023

View reviewed changes

test/test_utils.py Outdated Show resolved Hide resolved

jpfeuffer added 5 commits February 28, 2023 12:12

rearrange new test

f26f4ed

retrigger CI which is instable.. I do not have permissions to retrigger

0bcf881

forgot test prefix

2167739

depend on recipes fixture

28ed120

skip on osx

b599538

poshul mentioned this pull request Mar 3, 2023

conda builds not working anymore OpenMS/OpenMS#6514

Open

johanneskoester requested changes Mar 24, 2023

View reviewed changes

try to use repodata

06fd75c

do not instatiate RepoData, as the required methods are static

11f812c

johanneskoester self-assigned this Aug 9, 2023

Merge branch 'master' into jpfeuffer-patch-1

c8e0895

bgruening reviewed Nov 19, 2024

View reviewed changes

bioconda_utils/build.py Outdated Show resolved Hide resolved

bgruening added 2 commits November 19, 2024 16:13

Merge branch 'master' into jpfeuffer-patch-1

abf648d

Update bioconda_utils/build.py

386b376

coderabbitai bot reviewed Nov 19, 2024

View reviewed changes

bgruening approved these changes Nov 19, 2024

View reviewed changes

bgruening merged commit 6240d31 into bioconda:master Nov 19, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: success check does not consider pkg_dir #836 #838

fix: success check does not consider pkg_dir #836 #838

jpfeuffer commented Feb 24, 2023

jpfeuffer commented Feb 26, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023 •

edited

Loading

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 28, 2023

jpfeuffer commented Mar 3, 2023

timosachsenberg commented Mar 20, 2023

johanneskoester Mar 24, 2023

johanneskoester Mar 24, 2023

poshul commented Apr 1, 2023

jpfeuffer commented Apr 7, 2023

timosachsenberg commented Apr 14, 2023

poshul commented May 15, 2023

poshul commented Oct 25, 2023

timosachsenberg commented Nov 19, 2024

coderabbitai bot commented Nov 19, 2024 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

coderabbitai bot left a comment

bgruening left a comment

fix: success check does not consider pkg_dir #836 #838

fix: success check does not consider pkg_dir #836 #838

Conversation

jpfeuffer commented Feb 24, 2023

jpfeuffer commented Feb 26, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023 • edited Loading

daler commented Feb 27, 2023

jpfeuffer commented Feb 27, 2023

daler commented Feb 27, 2023

jpfeuffer commented Feb 28, 2023

jpfeuffer commented Mar 3, 2023

timosachsenberg commented Mar 20, 2023

johanneskoester Mar 24, 2023

Choose a reason for hiding this comment

johanneskoester Mar 24, 2023

Choose a reason for hiding this comment

poshul commented Apr 1, 2023

jpfeuffer commented Apr 7, 2023

timosachsenberg commented Apr 14, 2023

poshul commented May 15, 2023

poshul commented Oct 25, 2023

timosachsenberg commented Nov 19, 2024

coderabbitai bot commented Nov 19, 2024 • edited Loading

Walkthrough

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

coderabbitai bot left a comment

Choose a reason for hiding this comment

bgruening left a comment

Choose a reason for hiding this comment

jpfeuffer commented Feb 27, 2023 •

edited

Loading

coderabbitai bot commented Nov 19, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)