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

Rust plugin: rewrite the Rust plugin for core20 #4297

Merged
merged 2 commits into from
Sep 21, 2023
Merged

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Jul 30, 2023

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run pytest tests/unit?

This pull request migrates the Rust plugin to the new plugin system and also makes it more intelligent.

Three new options have been added:

  • One option for enabling LTO (more optimized binary)
  • One option for specifying Rust toolchain channel/version
  • One option for disabling default features in the Rust crate

@liushuyu liushuyu force-pushed the main branch 10 times, most recently from b8a4272 to b26285f Compare August 16, 2023 17:27
@liushuyu liushuyu marked this pull request as ready for review August 16, 2023 17:29
@liushuyu
Copy link
Contributor Author

Ready for review.

@cmatsuoka
Copy link
Contributor

It seems that this plugin can be added to https://github.com/canonical/craft-parts/ instead.

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2023

Codecov Report

Merging #4297 (20d2494) into main (dc58181) will decrease coverage by 0.07%.
The diff coverage is 64.70%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #4297      +/-   ##
==========================================
- Coverage   89.18%   89.11%   -0.07%     
==========================================
  Files         318      318              
  Lines       21125    21165      +40     
==========================================
+ Hits        18840    18862      +22     
- Misses       2285     2303      +18     
Files Changed Coverage Δ
snapcraft_legacy/plugins/v2/rust.py 69.84% <64.70%> (-25.82%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmatsuoka
Copy link
Contributor

Closing this PR, we have canonical/craft-parts#530.

@cmatsuoka cmatsuoka closed this Aug 16, 2023
@sergiusens sergiusens reopened this Aug 24, 2023
@liushuyu liushuyu changed the title Rust plugin: rewrite the Rust plugin using the new plugin system Rust plugin: rewrite the Rust plugin for core20 Sep 7, 2023
@liushuyu
Copy link
Contributor Author

liushuyu commented Sep 8, 2023

Rebased and re-targeted the pull request to do the core20 version of the plugin.

@liushuyu liushuyu force-pushed the main branch 3 times, most recently from 24308f2 to 0eafdb1 Compare September 10, 2023 20:52
@liushuyu
Copy link
Contributor Author

The tests should be fixed now.

@liushuyu liushuyu force-pushed the main branch 2 times, most recently from d5e49a6 to aa47593 Compare September 10, 2023 22:49
@cmatsuoka
Copy link
Contributor

@liushuyu We have some test errors

@liushuyu
Copy link
Contributor Author

@liushuyu We have some test errors

I don't quite understand why the CI is testing the new plugin system in the legacy plugin tests.

@cmatsuoka
Copy link
Contributor

I don't quite understand why the CI is testing the new plugin system in the legacy plugin tests.

V1 and V2 plugins are legacy, and the craft-parts plugins will be used for core22 only after updating to 1.25.1.

@liushuyu liushuyu force-pushed the main branch 4 times, most recently from b178095 to 56b9cee Compare September 11, 2023 18:27
@liushuyu
Copy link
Contributor Author

I don't quite understand why the CI is testing the new plugin system in the legacy plugin tests.

V1 and V2 plugins are legacy, and the craft-parts plugins will be used for core22 only after updating to 1.25.1.

I realized I had mocked the wrong function. I have fixed that now.

@liushuyu liushuyu requested a review from cmatsuoka September 12, 2023 05:25
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

This follows the mechanics of the craft-parts plugin. It deviates a bit from the regular v2 plugin which generally have less sophisticated possibilities regarding checking existing compilers, but in doing so it also reduces the impact in migrating to core22 or newer bases using the craft-parts plugin. We also need a spread test exercising rust part building using both V2 and craft-parts plugin. @liushuyu could you add such testing upon upgrading craft-parts to 1.25.1? @mr-cal, @sergiusens, any thoughts?

@cmatsuoka cmatsuoka requested a review from mr-cal September 12, 2023 13:00
snapcraft_legacy/plugins/v2/rust.py Outdated Show resolved Hide resolved
@mr-cal
Copy link
Collaborator

mr-cal commented Sep 14, 2023

Agreed, I think adding a spread test after craft-parts is upgraded is a good plan.

@liushuyu
Copy link
Contributor Author

Rebased against the latest main branch.

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Thanks!

... with the new one landed in the craft-parts
@mr-cal mr-cal merged commit b357ef5 into canonical:main Sep 21, 2023
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