-
-
Notifications
You must be signed in to change notification settings - Fork 591
refactor: make modules_mapping a regular rule #578
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
refactor: make modules_mapping a regular rule #578
Conversation
c03ec4c
to
385a983
Compare
Was there a reason why |
@kormide In the early days, the modules mapping was considerably faster compared to depending on all the wheels from |
385a983
to
54bc393
Compare
Signed-off-by: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com>
I was working on this a bit earlier since the previous version didn't work with Not really sure of a good way to benchmark it beyond rough timings and checking for cached actions, but it seems like it should be more efficient. Could also combine the json merger with the yaml generator if having them separate adds too much overhead, but I wasn't sure if there was another use for the merged json file beyond making the yaml one. I pushed the changes here. |
@corypaik - interesting approach. We have been using it on a large project and haven't felt the need to do this performance improvement since the testing to check whether the checked-in manifest is up to date or not doesn't require recomputing the whole file. The most expensive part of updating the modules mapping is downloading and, in some cases, compiling native modules. The tree traversal to generate the mappings is usually super fast and I don't think the performance improvement would be noticeable? |
I think making this a simple rule also fixed #574 because the same fetch packages can be now used here. So thanks for this change! |
PR Checklist
Please check if your PR fulfills the following requirements:
.par
files. See CONTRIBUTING.md for infoPR Type
What kind of change does this PR introduce?
What has changes
The current
modules_mapping
is a repository rule. This changeset moves it to be a regular rule, consuming the wheels from pip_install instead of re-downloading and building (when necessary) them twice.Does this PR introduce a breaking change?
Since the Gazelle plugin is not contained in any release, I don't consider it to be a breaking change.