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

tango feedstocks submission to conda-forge preparation #1

Merged
merged 27 commits into from
Jan 11, 2021
Merged

Conversation

beenje
Copy link
Owner

@beenje beenje commented Jan 7, 2021

This is a local PR to prepare submission to conda-forge.
This is based on the existing recipes from https://github.com/tango-controls/tango-conda-recipes and https://github.com/tango-controls/pytango-conda-recipes with some changes following the template from conda-forge.

@bourtemb, I added you as maintainer to the following recipes:

  • tango-idl
  • cpptango
  • tango-test
  • tango-admin

Is this OK? Shall I add someone else?
Could you please review those recipes? I bumped the build number compared to the existing recipes in tango channel. Not sure if we should or not.

@ajoubertza, I added you as maintainer to:

  • pytango
  • itango

Is this OK? Someone else to add?
For pytango, I removed the pytest command. In most recipes, the full test suite isn't run. But that's really up to the maintainer to decide. What do you think? Should we add it back?

@bourtemb
Copy link

bourtemb commented Jan 7, 2021

@beenje , thank you very much for what you're doing!

You can add me in the list of maintainers for the packages you listed for me, even though I haven't done anything on the tango-admin one yet.
If we could find some other volunteers, that would be great. Thank you for adding yourself in the list of maintainers.
Help is always welcome.

I would be in favour of keeping the pytest part for pytango.
I think it's good practice to test the packages, isn't it? Or would this be considered as too much and too slow?

@beenje
Copy link
Owner Author

beenje commented Jan 7, 2021

Thanks @bourtemb!
You are probably right about pytango. I added back the pytest command.
More testing is always good. It's actually not that long to run, so there is no good reason to skip it.

Yes, it would be nice to have more volunteers. Something to ask during next tango meeting?

Do you have an opinion about the build number? Should we just reset it to 0 as it's for a new channel? Or should we try to increase the number compared to the existing packages in tango-controls?

Copy link

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

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

Thanks @beenje for this great initiative.

I made a few comments on the Debug/Release mode and on appropriate licenses.

I think the licenses should be added to the upstream repositories (as already done for some of them like pytango) and not added here.

tango_admin install patch should rather be in tango_admin upstream repo.

recipes/cpptango/build.sh Outdated Show resolved Hide resolved
recipes/cpptango/meta.yaml Outdated Show resolved Hide resolved
recipes/itango/meta.yaml Outdated Show resolved Hide resolved
recipes/pytango/meta.yaml Outdated Show resolved Hide resolved
recipes/tango-admin/meta.yaml Outdated Show resolved Hide resolved
recipes/tango-admin/tango_admin-install.patch Outdated Show resolved Hide resolved
recipes/tango-idl/meta.yaml Outdated Show resolved Hide resolved
recipes/tango-test/build.sh Outdated Show resolved Hide resolved
recipes/tango-test/meta.yaml Outdated Show resolved Hide resolved
@bourtemb
Copy link

bourtemb commented Jan 7, 2021

Do you have an opinion about the build number? Should we just reset it to 0 as it's for a new channel? Or should we try to increase the number compared to the existing packages in tango-controls?

The safest approach is probably to increase the build number. I don't know how conda deals with this when you have the same package in different channels.

beenje and others added 8 commits January 7, 2021 13:19
Co-authored-by: Reynald Bourtembourg <[email protected]>
Co-authored-by: Reynald Bourtembourg <[email protected]>
Co-authored-by: Reynald Bourtembourg <[email protected]>
Co-authored-by: Reynald Bourtembourg <[email protected]>
Co-authored-by: Reynald Bourtembourg <[email protected]>
Allow to remove patch and local LICENSE file
@beenje
Copy link
Owner Author

beenje commented Jan 7, 2021

I'll make MR for missing LICENSE upstream. The files can stay in the recipe depending if we can make a new release or not.

@ajoubertza
Copy link

@ajoubertza, I added you as maintainer to:

  • pytango
  • itango

Is this OK? Someone else to add?

Thanks. I'm not sure of anyone else to add. Maybe @cpascual is interested?

For pytango, I removed the pytest command. In most recipes, the full test suite isn't run. But that's really up to the maintainer to decide. What do you think? Should we add it back?

I would like to keep the tests to ensure that the Conda build is working, as @bourtemb suggests. At least for Linux.

This PR doesn't cover Windows, but for future reference: the Windows the testing is more complicated and slower, so I'm OK with it being skipped - it gets done on AppVeyor, and we install binary wheels for Windows, so no additional compilation required.

@beenje
Copy link
Owner Author

beenje commented Jan 7, 2021

Do you have an opinion about the build number? Should we just reset it to 0 as it's for a new channel? Or should we try to increase the number compared to the existing packages in tango-controls?

The safest approach is probably to increase the build number. I don't know how conda deals with this when you have the same package in different channels.

I know that by default if a package is available in different channels, the one from the first defined channel (it has highest priority) will be used. I wasn't sure when build numbers are different, but I did a test. Even if foo_1 is available in channel A and foo_0 in channel B, what matters if the order of the channel.
So actually I think we should reset the build number to 0 for all packages.

@bourtemb
Copy link

bourtemb commented Jan 8, 2021

@beenje I created release 3.2 for TangoTest (https://github.com/tango-controls/TangoTest/releases/tag/3.2) which is including the LICENSE file.
For itango, we would need a new release too if possible.
@ajoubertza or another itango maintainer, could you make a new itango release, please, if it is in a reasonable state for a new release?
Latest release itango v0.1.7 is from October 2017. There has been new commits since that time.

@beenje
Copy link
Owner Author

beenje commented Jan 8, 2021

@beenje I created release 3.2 for TangoTest (https://github.com/tango-controls/TangoTest/releases/tag/3.2) which is including the LICENSE file.

Thanks. I bumped the conda recipe to 3.2.

For itango, we would need a new release too if possible.
@ajoubertza or another itango maintainer, could you make a new itango release, please, if it is in a reasonable state for a new release?
Latest release itango v0.1.7 is from October 2017. There has been new commits since that time.

For itango, I don't think we need a new release for the conda recipe. It's fine to have the LICENSE in the recipe as long as it has been submitted upstream.
After I agree that it would be good to make a new release of itango. I can look into it but would like to migrate to GitLab first so it can be done via CI.

@bourtemb
Copy link

bourtemb commented Jan 8, 2021

I made a new release (5.1.0) of tango-idl which also includes a LICENSE file.
LGPLv3 was used as originally suggested by @beenje in his PR and following comments on the current PR.

Remove local license
recipes/pytango/meta.yaml Outdated Show resolved Hide resolved
recipes/pytango/meta.yaml Outdated Show resolved Hide resolved
@beenje
Copy link
Owner Author

beenje commented Jan 11, 2021

@bourtemb and @ajoubertza I think I addressed all your comments. Do you agree or have something else to mention?

If you think the current recipes are in a good enough state, I propose to make a PR to conda-forge. I might rebase to keep one commit per recipe before that.

Copy link

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

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

@beenje , thanks again for this initiative.
This PR seems in a good shape to me.
I think you could start making a PR to conda-forge based on this work.
Rebasing to keep one commit per recipe before that seems like a good idea indeed.

@beenje beenje merged commit 5d4075c into tango Jan 11, 2021
beenje pushed a commit that referenced this pull request Jan 22, 2021
beenje pushed a commit that referenced this pull request Jul 23, 2021
beenje pushed a commit that referenced this pull request Jul 23, 2021
Fixed Test requirements and added noarch
beenje pushed a commit that referenced this pull request Aug 13, 2021
Add a patch to fix username
beenje pushed a commit that referenced this pull request Feb 19, 2022
Co-authored-by: Sherman J. Kisner <[email protected]>
beenje pushed a commit that referenced this pull request Apr 20, 2022
beenje pushed a commit that referenced this pull request Jun 14, 2022
REF: Minimal alias markup
beenje pushed a commit that referenced this pull request Oct 18, 2022
beenje pushed a commit that referenced this pull request Oct 18, 2022
@beenje beenje deleted the tango-prep branch October 22, 2022 16:09
beenje pushed a commit that referenced this pull request Nov 25, 2022
beenje pushed a commit that referenced this pull request Dec 1, 2022
Removed recipe (nebari) after converting into feedstock. [ci skip]
beenje pushed a commit that referenced this pull request Apr 14, 2023
beenje pushed a commit that referenced this pull request Apr 14, 2023
Update meta.yaml Pin Setuptools
beenje pushed a commit that referenced this pull request Apr 14, 2023
beenje pushed a commit that referenced this pull request Apr 14, 2023
Revise libcufile recipe / fix tests.
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