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

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

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

Conversation

jpfeuffer
Copy link
Contributor

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 jpfeuffer changed the title fix #836 fix: success check does not consider pkg_dir #836 Feb 24, 2023
@jpfeuffer
Copy link
Contributor Author

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

@daler
Copy link
Member

daler commented Feb 27, 2023

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

@jpfeuffer
Copy link
Contributor Author

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
Copy link
Member

daler commented Feb 27, 2023

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
Copy link
Contributor Author

jpfeuffer commented Feb 27, 2023

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
Copy link
Member

daler commented Feb 27, 2023

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
Copy link
Contributor Author

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
Copy link
Member

daler commented Feb 27, 2023

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 Outdated Show resolved Hide resolved
test/test_utils.py Outdated Show resolved Hide resolved
@jpfeuffer
Copy link
Contributor Author

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

@jpfeuffer
Copy link
Contributor Author

@daler Do you need anything else?

@timosachsenberg
Copy link

Hi, has there been any progress with this PR?

Comment on lines 121 to 125
platform = os.environ.get('OSTYPE', sys.platform)
if platform.startswith("darwin"):
platform = 'osx'
elif platform == "linux-gnu":
platform = "linux"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@poshul
Copy link

poshul commented Apr 1, 2023

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
Copy link
Contributor Author

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
Copy link

any update here?

@poshul
Copy link

poshul commented May 15, 2023

@johanneskoester @daler any updates on this?

@johanneskoester johanneskoester self-assigned this Aug 9, 2023
@poshul
Copy link

poshul commented Oct 25, 2023

@johanneskoester @daler any updates?

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.

5 participants