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.

Sign up for GitHub

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

Jump to bottom

Split check command into separate file #9707

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

Split check command into separate file #9707

wants to merge 1 commit into from

Conversation

Zopolis4
Copy link
Collaborator

@Zopolis4 Zopolis4 commented Apr 22, 2024 •
edited
Loading

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.

uberhacker
uberhacker reviewed Apr 23, 2024
View reviewed changes
lib/const.rb
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 5 times, most recently from 1783356 to 64ce216 Compare May 22, 2024 22:34
@Zopolis4 Zopolis4 marked this pull request as ready for review May 22, 2024 22:37
uberhacker
uberhacker reviewed Jun 16, 2024
View reviewed changes
README.md
@@ -94,7 +94,6 @@ Where available commands are:
| remove | remove package(s) |
| search | look for package(s) |
| sysinfo | show system information in markdown style |
| test | test crew command(s) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we want to test prior to submitting a PR? If this is removed, we would have to submit a PR and see if the tests fail first? If that is true, not ideal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we want to test prior to submitting a PR, we now run rake test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also allows easier testing on non-ChromeOS environments, as you do not need to have crew installed to test every command at once.

tests/cycle_test
abort "\n#{@failed} dependency #{@cycles} found.".lightred
else
puts "\nNo dependency cycles found.".lightgreen
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this no longer have any value? Someone put a lot of effort into creating this test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, it does not. As per the PR description, we have a number of packages with dependency cycles, including unavoidable ones such as cairo, harfbuzz, freetype and the rest.

If this test still worked, it would fail on behavior that we cannot change.

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 will live on in the git history, though 😅

@Zopolis4 Zopolis4 mentioned this pull request Jul 4, 2024
Merged
@Zopolis4 Zopolis4 requested a review from uberhacker July 7, 2024 04:38
@Zopolis4 Zopolis4 marked this pull request as draft September 12, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satmandu satmandu Awaiting requested review from satmandu

@supechicken supechicken Awaiting requested review from supechicken

@uberhacker uberhacker Awaiting requested review from uberhacker

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
merge conflict
Projects
None yet
Milestone
bin/crew refactoring
Development

Successfully merging this pull request may close these issues.
3 participants
@Zopolis4 @uberhacker @satmandu