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

Split check command into separate file #9707

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zopolis4
Copy link
Collaborator

@Zopolis4 Zopolis4 commented Apr 22, 2024

Split out check command to commands/check.rb.

Behavior changes:

  • Replace crew test with a Rakefile and crew check.
  • Rewrite the message printed when ruby_rubocop is not installed.
  • Remove the crew check -v behaviour.
    • Calling version.rb with the name of the package works the same, and dropping this wrapper allows us to drop the -V parameter which was at cross purposes with the same parameter when used like so: crew -V.
  • Make prop_test and buildsystem_test less verbose by default.
    • This also makes it a lot easier to read the unit test logs.
  • Fix the help message for crew check.
  • Remove cycle_test.
    • This test hasn't worked in quite some time, and we also have a number of packages with dependency cycles that we explicitly and knowingly added (i.e. CMake), so it doesn't add much value at this point.

Refactoring changes:
(prop_test and buildsystem_test)

  • Return literal '1' and '0' and add those rather than using @tofail.
  • Use CREW_PACKAGES_PATH and CREW_LIB_PATH rather than relative directory paths.
  • Take an actual package object in check_properties rather than using a name and an instance variable.
  • Use less single-use variables in and around check_buildsystem.

(bin/crew and commands/check.rb)

  • Merge check_package and copy_package into a single function, using the to_copy variable to replicate the previous method of outsourcing both the prompting for input and subsequent copying to copy_package.
    • As crew build, crew install and crew reinstall all now call Command.check, we rework the case of there being no local package to check to result in a silent ignore when we are calling it from another method (so you can install packages without having to worry about being in CREW_LOCAL_REPO_ROOT, but we let the user know about this happening when it is a call from crew check.
  • Use CREW_LIB_PATH instead of relative paths.
  • Use File.join instead of string interpolation.

Tested and working on x86_64.

Run the following to get this pull request's changes locally for testing.

CREW_REPO=https://github.com/Zopolis4/chromebrew.git CREW_BRANCH=projectregula8 crew update

@Zopolis4
Copy link
Collaborator Author

The unit tests are failing because the docopt string for crew build (and also crew install and crew reinstall) are broken and don't have anything for [-f|--force]. I'm not sure why this is only becoming an issue now, but on that note, does it even make sense to be running crew check when building, installing or reinstalling?

I get that crew build should have [-f|--force] because of the release/#{ARCH} stuff, and i'll add it because of that, but I'm questioning the logic behind running this when we build or install and so on.

crew build [options] [-k|--keep] [-v|--verbose] <name> ...
crew check [-V|--version] [-v|--verbose] <name> ...
crew build [options] [-f|--force] [-k|--keep] [-v|--verbose] <name> ...
crew check [-f|--force] <name> ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the verbose option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It never did anything, so removing it just makes things less misleading.

@Zopolis4 Zopolis4 added this to the bin/crew refactoring milestone May 13, 2024
@Zopolis4 Zopolis4 requested a review from uberhacker May 19, 2024 23:22
@Zopolis4 Zopolis4 force-pushed the projectregula8 branch 4 times, most recently from b584f9b to d5f85c3 Compare May 22, 2024 02:28
@Zopolis4 Zopolis4 marked this pull request as draft May 22, 2024 02:29
@Zopolis4 Zopolis4 force-pushed the projectregula8 branch 4 times, most recently from bebf062 to 1783356 Compare May 22, 2024 05:39
@Zopolis4 Zopolis4 marked this pull request as ready for review May 22, 2024 22:37
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.

None yet

2 participants