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

Include our package version in the manifest header #238

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Include our package version in the manifest header #238

merged 2 commits into from
Feb 14, 2017

Conversation

jparise
Copy link
Collaborator

@jparise jparise commented Feb 14, 2017

This gives us additional control over when to consider existing
artifacts as "stale" and in need of regeneration. At the moment, we
require a strict version string match, but as we mature in accordance
with semver guidelines, we can switch to a more fine-grained approach
based on Version.match?/3.

We don't need to bump the manifest version in this case because ...

  1. Reads against existing manifests will safely fail and result in
    desirable rebuild behavior.
  2. The manifest stuff is brand new, and it's unlike many folks are
    using it yet.

This gives us additional control over when to consider existing
artifacts as "stale" and in need of regeneration. At the moment, we
require a strict version string match, but as we mature in accordance
with semver guidelines, we can switch to a more fine-grained approach
based on Version.compare/2.

We don't need to bump the manifest version in this case because ...

1. Reads against existing manifests will safely fail and result in
   desirable rebuild behavior.
2. The manifest stuff is brand new, and it's unlike many folks are
   using it yet.
@@ -133,21 +133,29 @@ defmodule Mix.Tasks.Compile.Thrift do
end
end

defp thrift_vsn do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This name makes me think it's the actual name of the version of thrift supported (e.g., 0.9.3), which kind of opens up a can of worms... so far we seem to be 0.9.3 compatible and thrift seems to be backwards compatible, but does anyone know if they have made any statements about potential for backwards incompatibility? If thrift 1.0.0 was not backwards compatible with 0.9.x, how would we handle that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would it be clearer if I renamed this to package_vsn?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh I like that. It's accurate without being overly specific.

Copy link
Collaborator

@dantswain dantswain left a comment

Choose a reason for hiding this comment

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

My only comment was kind of beyond the scope of this PR except maybe changing the name of the function to avoid confusion over "version of elixir-thrift" vs "version of thrift".

@jparise jparise merged commit 87df071 into pinterest:thrift_tng Feb 14, 2017
@jparise jparise deleted the manifest-version branch February 14, 2017 19:09
@dantswain
Copy link
Collaborator

@jparise I just came across this today. We're using thrift_tng on a new project and have a couple devs working on it. When I updated today to pull in the TEnum meta function, it caused some ripple effects where people had to force a recompile of generated code (e.g., rm lib/generated/*).

So long story short, I was thinking it might be useful to actually tag the manifest with the commit SHA somehow?

@jparise
Copy link
Collaborator Author

jparise commented Feb 14, 2017

Yes, this is essentially #138. I'm not sure how much work we want to put into this system to support our current pre-release development environment, but I'm sure we can find some sort of compromise. Let's discuss more on that issue.

The current workaround is to mix thrift.compile --force whenever you update from the current in-development dependency.

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.

2 participants