Enable spring by default when executing packwerk init #153

alexevanczuk · 2021-10-21T21:01:23Z

Summary

This is a preamble to #150. As discussed in the thread, we want to make bin/packwerk init generate a spring-enabled bin so users can execute commands like bin/packwerk check and bin/packwerk update-deprecations more quickly.

Commits

change references to packwerk CLI command throughout
- This commit is all about changing the language in the docs to encourage people to use the generated binstub.
generate binstub along with other commands
- This commit is about generating the binstub along with the root package, configuration, and inflections

What are you trying to accomplish?

This is a pre-requisite to removing the need to maintain a list of custom inflections and load_paths, since we will soon be asking the rails application itself for these things on the fly. We want to make it easy for people to use spring with packwerk so this doesn't cause a performance degradation when running check or update-deprecations on single files or directories.

What approach did you choose and why?

In the short-term, we simply generate the binstub so spring helps make loading the rails application quicker.
In the long-term, if we can remove our dependency on zeitwerk (Shopify/constant_resolver#26), we can remove the need to load rails altogether.

What should reviewers focus on?

Making sure my understanding of how spring plays into things is correct.

Type of Change

Bugfix
New feature
Non-breaking change (a change that doesn't alter functionality - i.e., code refactor, configs, etc.)

Additional Release Notes

Breaking change (fix or feature that would cause existing functionality to change)

Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.

If no additional notes are necessary, delete this section or leave it unchanged.

The docs suggest using bin/packwerk. Users who do not have a binstub yet can generate one with bundle exec packwerk init

Checklist

I have updated the documentation accordingly.
I have added tests to cover my changes.
It is safe to rollback this change.

exterm · 2021-10-21T21:06:50Z

To add some context: This is a partial revert of #128

lib/packwerk/generators/templates/packwerk

alexevanczuk · 2021-10-27T18:57:09Z

Hey @rafaelfranca would you be able to provide any feedback on this approach to supporting spring with packwerk based on the context in #150?

exterm · 2021-11-01T20:01:50Z

lib/packwerk/generators/binstub.rb

+
+      private
+
+      def generate_packwerk_validate_script


this is not part of the revert, you added this, right? I wonder why we didn't need this before.

We also had the SpringCommand before, which I'm not quite sure what it does 🤷🏼

It was part of the revert:
It was called ApplicationValidation, which was generated when initializing packwerk here: https://github.com/Shopify/packwerk/pull/128/files#diff-ac5ae86965a90253f8d378798aa32f4041ec009d6b79c4420e88be6e67108e88L80

That application validation generation copied the binstub here: https://github.com/Shopify/packwerk/pull/128/files#diff-5cc7180b28a34feeec40fbab8714e5bb0dd61d6db8f1554084f63f9d2ca1e9b2L39

I just called it a binstub because that's really its main point. Before it was called ApplicationValidation I think because it was intended to be what you called when validating the application, but now it's what we will expect people to call always regardless of if validating or if running bin/packwerk update-deprecations, etc.

OK, that makes sense!

exterm · 2021-11-05T15:41:35Z

OK, so I just tried this out on our monolith. Findings:

be packwerk init generates a binstub, although I had to delete our existing binstub first. I think we had that one just for bundler integration, so that we didn't have to bundle exec packwerk.
New binstub wasn't executable, had to chmod +x. Not sure if we can do that automatically or what it was like when we last generated binstubs.
bin/packwerk check some_file.rb gives me

Version: 3.0.0

Usage: spring COMMAND [ARGS]

Commands for Spring itself:

  binstub         Generate Spring based binstubs. Use --all to generate a binstub for all known commands. Use --remove to revert.
  help            Print available commands.
  server          Explicitly start a Spring server in the foreground
  status          Show current status.
  stop            Stop all Spring processes for this project.

Commands for your application:

  rails           Run a rails command. The following sub commands will use Spring: console, runner, generate, destroy, test.
  rake            Runs the rake command
📦 Packwerk is inspecting 1 file
.
📦 Finished in 1.56 seconds

So, I assume that spring is not actually being used...

Maybe we do need to register a spring command, as the previous implementation did.

UPDATE: spring is started by running bin/packwerk check, but still, something is wrong there.

exterm · 2021-11-05T15:49:27Z

README.md

@@ -47,7 +47,7 @@ Or install it yourself as:

    $ gem install packwerk

-3. Run `packwerk init` to generate the configuration files
+3. Run `bundle exec packwerk init` to generate the configuration files


I'm curious, did this work without bundler before, and if so, how? 😄

yeah, init definitely doesn't work without bundle exec on master.

It works by having gem install packwerk. We have it in our Gemfile.

alexevanczuk · 2021-11-18T14:55:27Z

exe/packwerk

@@ -3,4 +3,10 @@

 require "packwerk"

-Packwerk::Cli.new(style: Packwerk::OutputStyles::Coloured.new).run(ARGV.dup)
+# Needs to be run in test environment in order to have test helper paths available in the autoload paths


This is what the previous binstub template did

alexevanczuk · 2021-11-18T15:01:07Z

After discussing at length with @exterm , we've made changes to the original implementation. I kept the old PR description for posterity.

Namely, we decided:
A) We leverage bundle binstub packwerk to generate the binstub. This is generally best practice and allows packwerk to support less and avoid pitfalls where other systems like bundler expect a certain type of binstub.
B) We use bin/spring binstub packwerk to "springify" the binstub
C) We require the user to require 'packwerk/spring_command' in config/spring.rb to actually properly register the packwerk spring command.

I tested this locally and it looks good in our application!

README.md

USAGE.md

exterm · 2021-11-18T20:11:51Z

test/unit/spring_command_test.rb

+      assert_equal("packwerk", command.exec_name)
+      assert_equal("packwerk", command.gem_name)
+      assert_equal("test", command.env(nil))
+    end


I think you copied this test over from the previous PR, right?

Do you think it is useful?

Yep, copied it over. I think it's probably not very useful.... but I think it's basically like a syntax + interface implementation test. This loads packwerk/spring_command so it will at least catch some simple mistakes. I think it can't hurt to keep.

I agree. The test is not very useful but as we don't expect the command to change often, it also doesn't incur a huge cost.

alexevanczuk · 2021-11-19T19:05:11Z

Modified this PR @exterm !

exterm · 2021-11-19T19:15:55Z

Just did a final test of bin/packwerk check startup times before and after this change by running on a single, very small ruby file within our monolith.

As packwerk check doesn't boot the application yet, I expected either no difference between the springified and non-springified binstub, or even the springified binstub to be a little slower.

However, these are the results:

springified binstub: ~3.5s
unspringified binstub: ~4.7s

I assume the speedup is from spring caching the bundle. Pretty cool.

exterm · 2021-11-19T19:18:00Z

I have looked over your changes again and they make perfect sense to me. If you squash the commits into some small number that makes sense, I think this is ready to merge! 👏🏼

alexevanczuk · 2021-11-19T19:21:13Z

Thanks @exterm -- I think a single commit makes sense. Just squashed, let me know if you agree or disagree and then I will land or make changes.

Excited to get this through. Next up is to rebase #150 off of this, then remove the custom inflections change as well, then cut a new release.

exterm · 2021-11-19T19:23:12Z

cut a new release

Sounds like packwerk 2.0 to me if I think about it - eliminating the application caches is a major (positive) change IMO. Although technically backwards compatible 🤔

I think a single commit makes sense.

👍🏼

alexevanczuk · 2021-11-19T19:48:34Z

Will need your help to merge @exterm

Only those with write access to this repository can merge pull requests.

exterm · 2021-11-19T20:23:23Z

yep, I was just running a full check of our monolith with the springified binstub to make sure this doesn't break any subtle edge cases - and then got pulled into other things.

The good news is that the run was successful 🎉

alexevanczuk requested a review from a team as a code owner October 21, 2021 21:01

exterm reviewed Oct 21, 2021

View reviewed changes

lib/packwerk/generators/templates/packwerk Outdated Show resolved Hide resolved

alexevanczuk force-pushed the ae-enable-spring-with-packwerk-init branch from ecb1f9f to f9f8638 Compare October 27, 2021 20:32

exterm reviewed Nov 1, 2021

View reviewed changes

exterm reviewed Nov 5, 2021

View reviewed changes

alexevanczuk force-pushed the ae-enable-spring-with-packwerk-init branch from db27121 to 7d753a9 Compare November 18, 2021 14:54

alexevanczuk commented Nov 18, 2021

View reviewed changes

alexevanczuk force-pushed the ae-enable-spring-with-packwerk-init branch 2 times, most recently from 9d9570c to 23f0916 Compare November 18, 2021 14:56

exterm reviewed Nov 18, 2021

View reviewed changes

README.md Show resolved Hide resolved

exterm reviewed Nov 18, 2021

View reviewed changes

USAGE.md Outdated Show resolved Hide resolved

exterm reviewed Nov 18, 2021

View reviewed changes

USAGE.md Outdated Show resolved Hide resolved

exterm reviewed Nov 18, 2021

View reviewed changes

exterm approved these changes Nov 19, 2021

View reviewed changes

Reenable spring and update documentation

3f3f2da

alexevanczuk force-pushed the ae-enable-spring-with-packwerk-init branch from e9272d0 to 3f3f2da Compare November 19, 2021 19:20

exterm merged commit ff7ad9e into Shopify:main Nov 19, 2021

alexevanczuk deleted the ae-enable-spring-with-packwerk-init branch November 19, 2021 21:02

alexevanczuk mentioned this pull request Nov 22, 2021

Find load_paths dynamically by asking Rails and eliminate cache in Packwerk::Configuration #150

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable spring by default when executing packwerk init #153

Enable spring by default when executing packwerk init #153

alexevanczuk commented Oct 21, 2021 •

edited

Loading

exterm commented Oct 21, 2021

alexevanczuk commented Oct 27, 2021

exterm Nov 1, 2021

alexevanczuk Nov 1, 2021

exterm Nov 5, 2021

exterm commented Nov 5, 2021 •

edited

Loading

exterm Nov 5, 2021

exterm Nov 5, 2021

technicalpickles Nov 12, 2021

alexevanczuk Nov 18, 2021

alexevanczuk commented Nov 18, 2021

exterm Nov 18, 2021

alexevanczuk Nov 19, 2021

exterm Nov 19, 2021

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 •

edited

Loading

exterm commented Nov 19, 2021

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 •

edited

Loading

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 •

edited

Loading

Enable spring by default when executing packwerk init #153

Enable spring by default when executing packwerk init #153

Conversation

alexevanczuk commented Oct 21, 2021 • edited Loading

Summary

Commits

What are you trying to accomplish?

What approach did you choose and why?

What should reviewers focus on?

Type of Change

Additional Release Notes

Checklist

exterm commented Oct 21, 2021

alexevanczuk commented Oct 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterm commented Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 • edited Loading

exterm commented Nov 19, 2021

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 • edited Loading

alexevanczuk commented Nov 19, 2021

exterm commented Nov 19, 2021 • edited Loading

alexevanczuk commented Oct 21, 2021 •

edited

Loading

exterm commented Nov 5, 2021 •

edited

Loading

exterm commented Nov 19, 2021 •

edited

Loading

exterm commented Nov 19, 2021 •

edited

Loading

exterm commented Nov 19, 2021 •

edited

Loading