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

Recompile generated code after elixir-thrift dep.update #138

Open
pguillory opened this issue Jan 3, 2017 · 5 comments
Open

Recompile generated code after elixir-thrift dep.update #138

pguillory opened this issue Jan 3, 2017 · 5 comments

Comments

@pguillory
Copy link
Collaborator

The current thrift.compile logic only recompiles output files based on their last_modified timestamp relative to their respective input files. However they may also need to be regenerated if elixir-thrift itself has changed. For example the client and server have generated user modules interoperating with static modules like Thrift.Client.BinaryFramed. If the latter changes, we need to make sure the former changes as well.

@jparise
Copy link
Collaborator

jparise commented Jan 3, 2017

I would like to add support for having the compiler task write a "manifest" that would track its generated files. That would gives us a mechanism to clean and regenerate output files. This would also be useful in the event that our code generator changes the names of the output files that it generates.

Separately, we could consider some sort of "version" that ties the generated files to a specific revision of the code generator. Some possible options:

  1. Use an incremental code version number. This would be the most specific way to flag codegen changes, but it's also error prone because a human needs to bump this value manually.
  2. Use the package version. This is nice because it is tied to the package release process, but it doesn't handle intra-release changes. We might be able to use a git revision instead.
  3. Use an internal BEAM version. Perhaps in combination with the above, we could compare BEAM module versions to detect changes to the code generator. This is probably too magical for use in practice and feels like it could produce both false-positives and false-negatives.

@pguillory
Copy link
Collaborator Author

Where would the manifest file reside, the project root? How would it handle manual compile.thrift invocations alongside automatic ones? Like would it append to the manifest? How do you clean out that stuff when you no longer use a particular .thrift file?

We could store metadata in generated files as comments at the top of the file.

# generated_at: 2017-01-1 12:34:56
# thrift_version: abc123
# etc...

Determining the thrift version is still an interesting problem. I like the idea of using the beam version. There are several modules under Elixir.Thrift.* though, would we need to check them all?

What if we look at the modification times of our beam files, rather than the version? Effectively:

find . -name "Elixir.Thrift.*.beam" -exec stat -f %m {} \; | sort | tail -n1

And when we do the stale check, take the most recent modification from those + the .thrift file in question. Kind of hacky though. But it would be nice to have something that works for us during development and also for end users, without having to remember to bump version numbers.

@jparise
Copy link
Collaborator

jparise commented Jan 6, 2017

Where would the manifest file reside, the project root?

The convention is to use Mix.Project.manifest_path, which falls under _build/{env}/. The fact that it's environment-specific might require some thought, but it also reminds us that it's possible for projects to specify environment-specific Thrift schemas (e.g. additional schemas for tests).

I don't have specific answers to your other questions yet. We'll need to prototype the different approaches to see which works for all of our use cases. The standard manifest / mix clean stuff works well for fully-compiled artifacts, but because our generated Thrift files are intermediates, we probably can't adopt all of the same patterns wholesale.

jparise added a commit that referenced this issue Feb 12, 2017
This is signifiant rewrite of the compiler task to support manifests,
making us a better mix citizen and adding support for `mix clean`-based
file cleanup.

Our manifest is written as a binary-encoded Erlang term to give us the
most forward-compatible file format. At the moment, we only store a
manifest version number and the list of generated files, but we may
expand this in the future to include additional dependency information
such as the "version" of the code that was used to generate these files.

See #138 for a more detailed discussion.
@jparise
Copy link
Collaborator

jparise commented Feb 14, 2017

We now determine "staleness" based on a manifest version and the package's version. This should work well for most users once we ship regular releases, but during development the package version number doesn't change often enough to trigger rebuilds when they might be necessary.

Possible options:

  1. Acknowledge that development versions are "special" and assume that users will run mix compile.thrift --force whenever they update the thrift dependency.
  2. Include the git revision (SHA1) in the manifest when we're running a development version.

I think (2) is reasonable, but it comes at the cost of more code paths. Probably worth it?

@jparise
Copy link
Collaborator

jparise commented Feb 15, 2017

I looked into including the git revision in the manifest header. It's a little bit trickier than I expected, but it's definitely possible. After hunting around the mix code a bit more, I'm left with the question as to whether we should:

  1. Run git rev-parse HEAD manually, from within our package directory (e.g. deps/thrift/) each time the compiler runs when we've already determined we're running from a -dev build.
  2. Use the revision information from our dependency's mix.lock entry, assuming that our including application is keeping that up-to-date. I don't believe this works with :path dependencies, however, so it might be a non-starter if that's an important case to cover.

I haven't been able to find any other way to extract version/revision information in a way that's available for all of our use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants