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

Support using cargo make as library #1112

Merged
merged 16 commits into from
Jul 17, 2024

Conversation

SamuelMarks
Copy link
Contributor

@SamuelMarks SamuelMarks commented Jul 12, 2024

Related to and closes #1110

src/lib/error.rs Outdated Show resolved Hide resolved
src/lib/types.rs Outdated Show resolved Hide resolved
@sagiegurari sagiegurari changed the base branch from master to 0.37.14 July 13, 2024 14:31
@sagiegurari
Copy link
Owner

@SamuelMarks amazing job thanks a lot. its actually something i wanted to do for a long time but didn't have the capacity so its great.
got few questions and also i see a test error. would love if you can take a look.

@SamuelMarks
Copy link
Contributor Author

@sagiegurari Great to hear you're open to this PR!

Let me take a look at the failing test and see if I can't resolve.

Copy link

codecov bot commented Jul 14, 2024

Codecov Report

Attention: Patch coverage is 85.19856% with 123 lines in your changes missing coverage. Please review.

Project coverage is 92.70%. Comparing base (4dfc585) to head (4564d61).
Report is 1 commits behind head on 0.37.14.

Files Patch % Lines
src/lib/error.rs 29.41% 24 Missing ⚠️
src/lib/runner.rs 77.41% 14 Missing ⚠️
src/lib/scriptengine/rsscript_test.rs 50.00% 11 Missing ⚠️
src/lib/execution_plan.rs 86.00% 7 Missing ⚠️
src/lib/descriptor/descriptor_deserializer.rs 45.45% 6 Missing ⚠️
src/lib/scriptengine/generic_script_test.rs 50.00% 6 Missing ⚠️
src/lib/functions/mod.rs 72.22% 5 Missing ⚠️
...rc/lib/scriptengine/duck_script/sdk/cm_run_task.rs 16.66% 5 Missing ⚠️
src/lib/installer/mod.rs 50.00% 4 Missing ⚠️
src/lib/cli_parser_test.rs 95.23% 3 Missing ⚠️
... and 24 more
Additional details and impacted files
@@             Coverage Diff              @@
##           0.37.14    #1112       +/-   ##
============================================
+ Coverage    69.27%   92.70%   +23.42%     
============================================
  Files          118      119        +1     
  Lines        26049    26203      +154     
============================================
+ Hits         18046    24292     +6246     
+ Misses        8003     1911     -6092     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sagiegurari
Copy link
Owner

@SamuelMarks your fixes solve the CI issue but i also merged #1109 which caused some merge conflicts. any chance you can resolve it so i'll push this? thanks

# Conflicts:
#	src/lib/cli.rs
#	src/lib/cli_commands/diff_steps.rs
#	src/lib/cli_commands/print_steps.rs
#	src/lib/cli_commands/print_steps_test.rs
#	src/lib/execution_plan.rs
#	src/lib/execution_plan_test.rs
#	src/lib/runner.rs
@SamuelMarks
Copy link
Contributor Author

@sagiegurari Resolved that merge conflict, still some weirdness going with this logic:
https://github.com/sagiegurari/cargo-make/blob/0119754/src/lib/execution_plan.rs#L491-L498

Once resolved let's try and merge this quickly before more PRs come through! - As this PR changes the whole codebase, adding error handling &etc.

@sagiegurari
Copy link
Owner

thanks. there are no PRs at the moment so i wouldn't worry about it.
as for the legacy task, i'm planning to remove it soon i hope.

@SamuelMarks
Copy link
Contributor Author

SamuelMarks commented Jul 14, 2024

Cool cool; no I mean 18 tests are failing because of that logic I linked. What do you want that logic to be? - Silent ignore? - Ignore but give a warning message? - Something else?

EDIT: Committed up code which comments out the return Err(CargoMakeError::NotFound lines in build of src/lib/execution_plan.rs

…eError::NotFound` on `skip_init_end_tasks` & `skip_init_end_tasks` & `end_task`
@sagiegurari
Copy link
Owner

I'll review the updated code tomorrow. if it makes a fuzz, drop the legacy migration task from the codebase as i want to delete it anyway.

… enable library authors to expand the spec (* might need some post-processing)
@sagiegurari sagiegurari merged commit 8b07c69 into sagiegurari:0.37.14 Jul 17, 2024
12 checks passed
@sagiegurari
Copy link
Owner

@SamuelMarks merging. thanks a lot

@sagiegurari
Copy link
Owner

@SamuelMarks while merging it to master (and planning to release it), i saw that there is an unsafe block and its crashing in mac OS in the ci cd flow.
please take a look

https://github.com/sagiegurari/cargo-make/actions/runs/9970773762

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.

cargo-make as library: task exection in asynchronous persistent queues
2 participants