Skip to content

Add renode-cli #28944

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

Merged
merged 78 commits into from
Mar 26, 2025
Merged

Add renode-cli #28944

merged 78 commits into from
Mar 26, 2025

Conversation

MementoRC
Copy link
Contributor

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

Copy link
Contributor

Hi! This is the staged-recipes linter and your PR looks excellent! 🚀

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/renode-cli/recipe.yaml) and found some lint.

Here's what I've got...

For recipes/renode-cli/recipe.yaml:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: generic.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12979430354. Examine the logs at this URL for more detail.

@MementoRC MementoRC mentioned this pull request Jan 26, 2025
10 tasks
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Jan 27, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/renode-cli/recipe.yaml, recipes/renode-cores/recipe.yaml) and found it was in an excellent condition.

@danielnachun
Copy link
Contributor

Since build.sh here is mostly a wrapper dotnet publish, I'm wondering if rather than trying to patch that script, if we can just figure out the right dotnet publish commands and call those directly. This usually leads to a much simpler, easier to maintain recipe.

@danielnachun Please take a look at: #28944 - The recipe has some 'gcc' compilations that seem to integrate into DLLs, does that mean that it cannot be a noarch? Is dotnet that good of a magician?

I believe the native code originates from https://github.com/antmicro/tlib/tree/4a28cc4048ebcf4196a110218adff13253f1ba69. Normally I would say that we should build this as a separate package and make it a dependency. However, in this case that repository is untagged and is itself a maintenance fork of another untagged repository. Ideally we would built this a split package with tlib as a native package and recode-cli as a noarch package that depends on it. I don't have much experience making split packages but this one seems fairly simple.

Let me know if you have questions so far, and thanks for being flexible about how to do this. We're still figuring out how to handle .NET best in conda-forge so there's a lot of best practices that still need to be worked out, especially for more complex packages like this one.

@MementoRC
Copy link
Contributor Author

Sounds good, but then I need to go back to the drawing board. I made a few split packages and will give it a try. I will ping the upstream maintainer to try to get a tag, they seem very thorough and conda-friendly - It's curfew over here, so I'll follow-up tomorrow

@danielnachun
Copy link
Contributor

Great! I'll keep an eye on this issue in case you run into more challenges but this is great progress so far!

@MementoRC
Copy link
Contributor Author

MementoRC commented Jan 28, 2025

@danielnachun A little bit of butchering, but what are your thoughts so far? We can't split as rattler-build cache feature is experimental

@MementoRC
Copy link
Contributor Author

MementoRC commented Jan 30, 2025

@danielnachun Now would be a good time to peak-a-boo! I tried to add a dynamic library loader to the code to load the *.so from the various renode-cores in the renode-cli. I don't know if it works.
I don't know why the noarch triggers a non-unix build nonetheless, so it fails to build renode-cli on non-unix

@danielnachun
Copy link
Contributor

This is looking better but I am still kind of concerned at the amount of patching and modifications we're making here. Some of this may be unavoidable, but right now this still seems difficult to maintain without a really deep understanding of this tool.

A couple thoughts so far:

  • If we have to write a custom dynamic loader for the native core plugins just to get a noarch package, we should probably just accept this being a native package now. I wasn't aware of those plugins when I initially suggested trying noarch, but I would rather we just try to modify the code and build system as little as possible.
  • Have you tried the dotnet publish command yet? I haven't had a chance to try it yet myself, but I'm thinking we'd need a lot less modification if we can just patch enough for that to work.

As I've mentioned before, this is a work in progress, but from what I've dealt with so far, things that are easy and reasonable to patch are:

  • .NET version requirements
  • Dependency versions if a version is outdated and causing a build to fail
  • Forced usage of Mono on Linux and macOS

Making changes beyond those basic things runs a much higher risk of breaking automated updates. This weekend I will take what you have and try to simplify as much as possible. Thanks for your patience so far - I want to figure this out in a way that really makes it easy to maintain in the future!

@MementoRC MementoRC requested a review from danielnachun March 20, 2025 18:50
Copy link
Contributor

@danielnachun danielnachun left a comment

Choose a reason for hiding this comment

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

renode-cli looks great now. I have a few questions about renode-cores and then I think we'll be ready to go.

@MementoRC MementoRC requested a review from danielnachun March 21, 2025 18:37
Copy link
Contributor

@danielnachun danielnachun left a comment

Choose a reason for hiding this comment

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

I made some very minor comments for a few things I didn't catch in my last review. Once those are done I think we'll be all set.

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/renode-cli/recipe.yaml, recipes/renode-python-tools/recipe.yaml, recipes/renode-cores/recipe.yaml) and found some lint.

Here's what I've got...

For recipes/renode-python-tools/recipe.yaml:

  • noarch packages can't have skips with selectors. If the selectors are necessary, please remove noarch: python.

For recipes/renode-python-tools/recipe.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.
    • For the test.requires section of recipe, you should usually use python ${{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/14010391869. Examine the logs at this URL for more detail.

@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Mar 22, 2025

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/renode-cores/recipe.yaml, recipes/renode-cli/recipe.yaml) and found it was in an excellent condition.

@MementoRC MementoRC requested a review from danielnachun March 25, 2025 15:22
@MementoRC MementoRC requested a review from danielnachun March 25, 2025 21:09
Copy link
Contributor

@danielnachun danielnachun left a comment

Choose a reason for hiding this comment

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

@ocefpaf we can merge this now! @MementoRC Thank you for your patience in putting up with my pickiness here. This is an incredibly complex recipe so it's essential that we follow the strictest standards to have any hope of being able to maintain it long term, and I think it's as simple as we can make it for now.

@MementoRC
Copy link
Contributor Author

@ocefpaf we can merge this now! @MementoRC Thank you for your patience in putting up with my pickiness here. This is an incredibly complex recipe so it's essential that we follow the strictest standards to have any hope of being able to maintain it long term, and I think it's as simple as we can make it for now.

Thank you for your time and help, I learned a lot.

@ocefpaf ocefpaf merged commit 2f4db5c into conda-forge:main Mar 26, 2025
7 checks passed
@MementoRC MementoRC deleted the renode-cli branch April 1, 2025 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants