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

adding peru override watch #163

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

adding peru override watch #163

wants to merge 1 commit into from

Conversation

spasarok
Copy link
Collaborator

Hello! I made a peru override watch command to automatically sync overrides when they change. I was wondering you would be willing integrate this or something similar into Peru. It would be super helpful for my and some of my teammates' workflows. Thanks!

@spasarok
Copy link
Collaborator Author

Not sure what checks are failing, but I'm happy to change/fix whatever you want.

@oconnor663
Copy link
Member

oconnor663 commented Jul 17, 2016

Thanks for the PR! I'm just starting to take a look, but it looks like the test failures are actually just lint warnings:

peru/main.py:210:1: E304 blank lines found after function decorator
peru/runtime.py:7:1: F401 'sys' imported but unused
peru/runtime.py:150:29: E128 continuation line under-indented for visual indent
peru/runtime.py:151:29: E128 continuation line under-indented for visual indent
peru/runtime.py:185:1: W293 blank line contains whitespace
peru/runtime.py:195:80: E501 line too long (87 > 79 characters)
peru/runtime.py:197:1: W293 blank line contains whitespace
ERROR: flake8 returned an error code

If you run test.py locally, it should run the exact same tests/lints for you. (Or complain that you don't have flake8 installed.)

@oconnor663
Copy link
Member

Could you please tell me more about your workflow? I know almost nothing about how people use override in the real world, and I'm really interested to learn more.

@oconnor663
Copy link
Member

My initial reaction is that I'd prefer to avoid taking this sort of dependency inside peru itself, if we could handle the same sort of use case with external tools. Would it be possible to handle your workflow with a Watchman trigger? Triggering build commands was the first use case Watchman was designed for, so I expect it would work very well with peru. Filesystem watching tends to have a lot of complications and corner cases, and I'd be curious how watchdog and Watchman compare in cases like rapid updates to many files:

Watchman waits for the filesystem to settle before processing any triggers, batching the list of changed files together before invoking the registered command.

and atomic-file-moving-instead-of-in-place-editing:

Vim does not modify files unless directed to do so. It creates backup files and then swaps them in to replace the files you are editing on the disk. This means that if you use Vim to edit your files, the on-modified events for those files will not be triggered by watchdog. You may need to configure Vim to appropriately to disable this feature.

Very curious to hear your thoughts on those questions, @spasarok. Also @olson-sean-k.

Apart from the broad discussion above, I'll go ahead and comment inline with feedback for this PR specifically.

logging.info('moved %s: from %s to %s', what, event.src_path,
event.dest_path)
logging.info('running peru sync --force')
os.system('peru sync --force')
Copy link
Member

Choose a reason for hiding this comment

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

We'd probably want some way to control whether syncs are done with --force or not. Maybe with a --force flag on the watch command?

Copy link
Member

Choose a reason for hiding this comment

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

Rather than shelling out with os.system, we'd probably want to call directly into imports.checkout the way that do_sync does. That would mean that these functions would need to be coroutines.

Copy link
Member

@oconnor663 oconnor663 Jul 17, 2016

Choose a reason for hiding this comment

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

Peru doesn't currently do any kind of filesystem locking to prevent running multiple commands in parallel in the same project. But if you do that, it could cause weird conflicts and errors. We should definitely add proper locking at some point, but it still also probably makes sense for a watcher to avoid queueing up several waiting instances at once (or running commands that will error out, depending on how we want to implement lock contention).

@spasarok
Copy link
Collaborator Author

Hello! Thanks for the discussion. I'll respond in order.

Could you please tell me more about your workflow? I know almost nothing about how people use override in the real world, and I'm really interested to learn more.

We use Peru for WordPress development. WordPress uses themes to style sites and plugins to provide functionality independent of a site theme. Each site that we build has a general repo with the site theme and we use Peru to sync plugins and other dependencies. This workflow is great for most dependencies, but sometimes we're syncing plugins that we're still in the middle of developing. In these cases we have to run peru sync pretty continuously in order to test changes to whatever plugin we're developing. This can be tedious and break up the workflow, especially when testing many small atomic changes in quick succession. I was thinking that it could be useful to have a watch command for Peru, sort of like the watch command that Compass uses to compile Sass to CSS (http://compass-style.org/help/). This would allow the workflow for testing dependency development to go from text editor -> terminal -> browser to simply text editor -> browser.

@spasarok
Copy link
Collaborator Author

Would it be possible to handle your workflow with a Watchman trigger?

I'm happy to look into this.

@spasarok
Copy link
Collaborator Author

We'd probably want some way to control whether syncs are done with --force or not. Maybe with a --force flag on the watch command?

Forcing can definitely use a flag instead. I grappled with this topic for a bit, but I ultimately decided to force --force for the following reasons:

  • If you're watching a dependency for changes so that you can sync those changes to your development environment, then overwriting whatever's already in that development environment is an implied goal.
  • I modeled this off Compass's watch command, which by default updates the environment resource whenever the components change. So, anyone familiar with this sort of watch system might be confused if watch does not automatically sync with force.

However, I agree that it's much safer to simply require --force than to make the assumptions above, so I'm totally on board with that.

@spasarok
Copy link
Collaborator Author

Rather than shelling out with os.system, we'd probably want to call directly into imports.checkout the way that do_sync does. That would mean that these functions would need to be coroutines.

Sounds good. I ran into some issue trying to use imports.checkout in the runtime file, but I'll delve into it a little more.

@spasarok
Copy link
Collaborator Author

The display object (usually runtime.display) has a print method that lets us do regular printing in a way that doesn't screw up the redrawing.

Sweet, I'll take a look at it.

@oconnor663
Copy link
Member

Very cool. I bet Watchman is going to be perfect for this. It'll also be more flexible if your workflow grows in the future to include codegen/compilation/restarting before or after the peru sync.

Either way, I'm going to need to get back to #115 at some point, to get ahead of some of the cache corruption issues that might come up here with multiple peru processes running.

@matro
Copy link

matro commented Aug 6, 2016

Hey @oconnor663, co-worker of @spasarok here.

We're starting to use Modd for our general watching/building needs. At a glance, it looks to me like it it fills the same niche as Watchman. We've only got it in 4 or 5 projects so far, so switching to Watchman might be something we'd do. But to maybe shed some light on how we're using overrides and how our current watching works, here're a couple of snippets from some of our modd.confs:

peru.yaml .peru/overrides/* {
    prep: peru sync
}

When either peru.yaml changes (including when switching branches in the codebase), we just run a regular vanilla peru sync. If a peru override add or peru override delete happens, same deal.

This is great but we don't know of anything to watch in the Peru-using repo's world that'd tell us if the override's content changes. My first thought is using symlinks in .peru/ somewhere, but that feels like a bad idea to me, and I don't know if Modd or other programs will even follow them.

With other programs like Compass or MkDocs that have a built-in watcher, we tell Modd to delegate its watching like this:

mkdocs.yml {
    prep: mkdocs build
    daemon: mkdocs serve
}

In this quasi-contrived case, it runs mkdocs build on startup and whenever mkdocs.yml changes, and then it also runs mkdocs serve for its .md-file-watching and browser-auto-refreshing local webserver goodness. For some pattern like wp-content/themes/SiteName/**/*.scss we'd have a compass watch daemon.

So if Peru had a built-in watch command (or some wrapper script would work too), we'd be able to faux-daemonize it like this...

peru.yml {
    daemon: peru watch
}

...or...

.peru/overrides/* {
    daemon: peru override watch
}

...and we'd have one fewer thing to manually manage in our workflow.

@olson-sean-k
Copy link
Member

I'd really like to leave this to external tools, and it seems like overrides are the only tricky part. The names and paths of overrides are trivial to query, so I think the right way to support this may be to provide complete and machine-readable output about overrides for external tools to consume.

For example, we could provide a --json option that can be used with peru override list that prints names and absolute paths of all known overrides as JSON. An external tool could use this to query the paths where overrides live and watch them.

Machine-readable output for query commands may be useful in general (see #165), so we could consider providing a --json option for other commands as well to ease automation. The output format is of course debatable. 😃

I'm not sure what this would mean for your modd configuration, since something would have to notice override changes and update the paths that are being watched. It may still be necessary to watch .peru/overrides to know when to update the override paths, which is a bit of a hack. This actually makes me question using completely different interfaces for modules and overrides...

@oconnor663
Copy link
Member

By the way, if you guys would like to schedule a group video chat or something like that to brainstorm about this stuff, I'd be up for it.

@matro
Copy link

matro commented Aug 9, 2016

@olson-sean-k Yeah, overrides are the only tricky part that we've gotten completely stuck on here. I think a --json option would definitely get us unstuck. It'd also definitely relieve pressure for things like #165, I'd bet.

Not much would change in our modd.confs. There'd just be a daemon: in our existing peru.yaml, .peru/overrides/ {} block that runs some script, rather than running peru directly.

@matro
Copy link

matro commented Aug 9, 2016

I feel like the question of whether or not this kind of functionality is appropriate for Peru itself comes down to... how much "batteries included" does Peru want to be?

@oconnor663 I'd definitely be up for a group chat to brainstorm with you and @spasarok sometime.

@spasarok
Copy link
Collaborator Author

spasarok commented Aug 9, 2016

Sure, I'm down for a group chat. @oconnor663 and @olson-sean-k what time works best for you?

@matro matro mentioned this pull request Aug 9, 2016
@oconnor663
Copy link
Member

We did it! https://youtu.be/YxSsRztjMec

My notes. Please add anything I left out. TODO new peru features in bold.

  • @spasarok ran into some nasty conflicts with Python 2 on Ubuntu. We need to improve the docs after we figure out what's up there.
  • For overrides and file watching, @matro's going to try making some wrapper commands that add an override and establish a watcher at the same time. (We could document this sort of thing, like we've done with Make.)
  • Defining new modules and importing them programmatically is kind of a can of worms, because we don't have a round-trip YAML parser/emitter that preserves comments and formatting. As long as this is something that humans are doing manually, we'll see if we can convince them to keep doing it in vim instead of bash.
  • Having a peru module [list] or something like that on the command line would make it easier to script reups that make a separate commit for each module. @spasarok and I will keep hacking on adding module list command #165.
    • A --json flag for that command, and also for peru override [list], will make life easier when someone inevitably tries to put a null character in a module name, or whatever other nonsense. @olson-sean-k, you wanna take this one? :-D
  • People have asked for tags / sync profiles / filters / something, to handle things like dev-only dependencies. There are a lot of different ways this could work. Probably the easiest thing will be to think about how we would want peru.yaml to look in this world (or whether it might split into more than one file), and work backwards from there.
  • _PERU CON 2016_

@oconnor663
Copy link
Member

Since I've already got some general future plans written down in this issue, I'm just going to add more here :) Takeaways from our meetup yesterday, other folks please add more:

  • It probably makes sense to have PERU_CACHE_DIR be in your HOME by default.
  • Building with lots of overrides might be unnecessarily slow. Example project requested for testing.

@oconnor663
Copy link
Member

@olson-sean-k just added the --json flag for peru override [list] in 20bf6fd.

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.

4 participants