/rebar

Move include/rebar.hrl to src/rebar.hrl #281

Merged
merged 1 commit into from Jun 14, 2014

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
8 participants
@tuncer @ferd @sebastian @tsloughter @AeroNotix @seth @puzza007 @jaredmorrow
@tuncer
Contributor

tuncer commented May 22, 2014

rebar.hrl is only meant to be used by src/*.
@tuncer
Move include/rebar.hrl to src/rebar.hrl 
rebar.hrl is only meant to be used by src/*.
bf63171
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd May 22, 2014

Contributor

Are we aware of any project that decided to depend on this and would break with it? If not (and I guess not), that looks fairly good/safe.

Contributor

ferd commented May 22, 2014

Are we aware of any project that decided to depend on this and would break with it? If not (and I guess not), that looks fairly good/safe.
@tuncer

This comment has been minimized.

Show comment Hide comment
@tuncer

tuncer May 22, 2014

Contributor

I'm not aware of any, and for example all plugins I've seen are calling rebar_log:log/3 directly. Also see #104.

Contributor

tuncer commented May 22, 2014

I'm not aware of any, and for example all plugins I've seen are calling rebar_log:log/3 directly. Also see #104.
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd May 22, 2014

Contributor

+1 then.

Contributor

ferd commented May 22, 2014

+1 then.

tsloughter added a commit that referenced this pull request Jun 14, 2014

@tsloughter
Merge pull request #281 from tuncer/rebar-h-internal 
Move include/rebar.hrl to src/rebar.hrl
07e2232

@tsloughter tsloughter merged commit 07e2232 into rebar:master Jun 14, 2014
1 check passed

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@tuncer tuncer deleted the tuncer:rebar-h-internal branch Jun 14, 2014

@jaredmorrow jaredmorrow referenced this pull request Jun 15, 2014

Merged

Fix #249 (erlc regression) #285

@sebastian

This comment has been minimized.

Show comment Hide comment
@sebastian

sebastian Jun 20, 2014

Hello devs,

First of all, thank you for providing and maintaining a great tool that we all rely on!

Now on to the less pleasant parts:

There are actually some tools relying on the rebar.hrl in the include lib.
Here are the ones I found on github: https://github.com/search?q=-include_lib%28%22rebar%2Finclude%2Frebar.hrl%22%29.&ref=cmdform&type=Code

I noticed the change since one of my dependencies broke, and am in the process of submitting fixes where needed.

I am not sure if you are using semantic versioning? But if you were, then this would actually require a new major version, as it breaks backwards compatibility. At least it is my impression that headers in the include lib are considered to be part of a public API?

You have since tagged a version 2.4 (from 2.3.X), and I would have expected that you should then at least have included some deprecation warnings in the include/rebar.hrl, for then to remove it at a later time?

sebastian commented Jun 20, 2014

Hello devs,

First of all, thank you for providing and maintaining a great tool that we all rely on!

Now on to the less pleasant parts:

There are actually some tools relying on the rebar.hrl in the include lib.
Here are the ones I found on github: https://github.com/search?q=-include_lib%28%22rebar%2Finclude%2Frebar.hrl%22%29.&ref=cmdform&type=Code

I noticed the change since one of my dependencies broke, and am in the process of submitting fixes where needed.

I am not sure if you are using semantic versioning? But if you were, then this would actually require a new major version, as it breaks backwards compatibility. At least it is my impression that headers in the include lib are considered to be part of a public API?

You have since tagged a version 2.4 (from 2.3.X), and I would have expected that you should then at least have included some deprecation warnings in the include/rebar.hrl, for then to remove it at a later time?
@tsloughter

This comment has been minimized.

Show comment Hide comment
@tsloughter

tsloughter Jun 20, 2014

Member

@sebastian thanks, that was a major screw up on our part. We should have at least searched github to check and you are right about the major version bump as well.

We will work to do better in the future.

Member

tsloughter commented Jun 20, 2014

@sebastian thanks, that was a major screw up on our part. We should have at least searched github to check and you are right about the major version bump as well.

We will work to do better in the future.
@AeroNotix

This comment has been minimized.

Show comment Hide comment
@AeroNotix

AeroNotix Jun 20, 2014

Another reason that #301 is a good thing.

AeroNotix commented Jun 20, 2014

Another reason that #301 is a good thing.
@sebastian

This comment has been minimized.

Show comment Hide comment
@sebastian

sebastian Jun 20, 2014

Most of all I am very grateful that you are all putting in the effort to maintain the tool! So my thanks for that!

sebastian commented Jun 20, 2014

Most of all I am very grateful that you are all putting in the effort to maintain the tool! So my thanks for that!

sebastian added a commit to sebastian/rebar_proper_plugin that referenced this pull request Jun 20, 2014

@sebastian
Re-define now missing rebar.hrl logging macro's 
As of this recent pull-request to rebar/rebar:
rebar/rebar#281,
rebar/include/rebar.hrl no longer exists.

Since this project doesn't depend on any particular
version of rebar, and doesn't build with the
latest version from master, I have
reimplemented the logging macros that were previously
taken from the include file in the source itself.
436ed57

@sebastian sebastian referenced this pull request in andrzejsliwa/rebar_proper_plugin Jun 20, 2014

Closed

Re-define now missing rebar.hrl logging macro's #1

@sebastian

This comment has been minimized.

Show comment Hide comment
@sebastian

sebastian Jun 20, 2014

And just to complicate things a little: I am actually not sure how a change like this could have been communicated to actually reach me, since I am not an active participant in neither project nor the main erlang mailing lists.

In any case: have a great weekend folks!

sebastian commented Jun 20, 2014

And just to complicate things a little: I am actually not sure how a change like this could have been communicated to actually reach me, since I am not an active participant in neither project nor the main erlang mailing lists.

In any case: have a great weekend folks!
@AeroNotix

This comment has been minimized.

Show comment Hide comment
@AeroNotix

AeroNotix Jun 20, 2014

@sebastian you should use a known-good version of rebar to compile your project instead of depending on the tip for a start. :)

AeroNotix commented Jun 20, 2014

@sebastian you should use a known-good version of rebar to compile your project instead of depending on the tip for a start. :)
@sebastian

This comment has been minimized.

Show comment Hide comment
@sebastian

sebastian Jun 20, 2014

@AeroNotix I couldn't agree more! The problem in this case was that rebar isn't at all a direct dependency of ours. A dependency of ours in turn does depend on master, but that is somewhat outside our control.

We are using @seth's excellent rebar_lock_deps_plugin to try to keep versions as locked down as possible, but unfortunately things still break sometimes.

sebastian commented Jun 20, 2014

@AeroNotix I couldn't agree more! The problem in this case was that rebar isn't at all a direct dependency of ours. A dependency of ours in turn does depend on master, but that is somewhat outside our control.

We are using @seth's excellent rebar_lock_deps_plugin to try to keep versions as locked down as possible, but unfortunately things still break sometimes.
@AeroNotix

This comment has been minimized.

Show comment Hide comment
@AeroNotix

AeroNotix Jun 20, 2014

That plugin is actively lying to people who use it. It's better than nothing, but we should not pretend that it provides repeatable builds.

AeroNotix commented Jun 20, 2014

That plugin is actively lying to people who use it. It's better than nothing, but we should not pretend that it provides repeatable builds.
@seth

This comment has been minimized.

Show comment Hide comment
@seth

seth Jun 20, 2014

While I agree that the lock deps stuff is not enough for reproducible builds, can you clarify the active lying part?

seth commented Jun 20, 2014

While I agree that the lock deps stuff is not enough for reproducible builds, can you clarify the active lying part?
@AeroNotix

This comment has been minimized.

Show comment Hide comment
@AeroNotix

AeroNotix Jun 20, 2014

@seth well the name implies that it "locks" something, when it doesn't really. It just figures out the commit you have and then... what? Source dependencies are probably not the right thing to do.

AeroNotix commented Jun 20, 2014

@seth well the name implies that it "locks" something, when it doesn't really. It just figures out the commit you have and then... what? Source dependencies are probably not the right thing to do.
@seth

This comment has been minimized.

Show comment Hide comment
@seth

seth Jun 20, 2014

So the plugin generates a rebar.config.lock file containing the SHA of all deps (including transitive). You can then use this config file for doing builds. As long as those repos and SHAs don't disappear (which they absolutely can!) you get the same code.

Is it perfect? Nope. Is source deps The Right Thing? Nope. Does it help solve a problem for some folks? Yep.

As for this change: this MUST be a major version change.

seth commented Jun 20, 2014

So the plugin generates a rebar.config.lock file containing the SHA of all deps (including transitive). You can then use this config file for doing builds. As long as those repos and SHAs don't disappear (which they absolutely can!) you get the same code.

Is it perfect? Nope. Is source deps The Right Thing? Nope. Does it help solve a problem for some folks? Yep.

As for this change: this MUST be a major version change.
@seth

This comment has been minimized.

Show comment Hide comment
@seth

seth Jun 20, 2014

So what's the plan on this header file? Revert the change and keep it public or major version bump?

seth commented Jun 20, 2014

So what's the plan on this header file? Revert the change and keep it public or major version bump?
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd Jun 20, 2014

Contributor

My guess is we should probably revert it, but I'm in a rush at work and haven't discussed it with anybody else.

Contributor

ferd commented Jun 20, 2014

My guess is we should probably revert it, but I'm in a rush at work and haven't discussed it with anybody else.
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd Jun 20, 2014

Contributor

@tuncer @tsloughter @jaredmorrow any opinion here?

Contributor

ferd commented Jun 20, 2014

@tuncer @tsloughter @jaredmorrow any opinion here?
@tuncer

This comment has been minimized.

Show comment Hide comment
@tuncer

tuncer Jun 22, 2014

Contributor

It appears that there are actually projects using rebar.hrl, so we should restore it for now, but we have to approach #104 sooner than later.

Contributor

tuncer commented Jun 22, 2014

It appears that there are actually projects using rebar.hrl, so we should restore it for now, but we have to approach #104 sooner than later.
@puzza007

This comment has been minimized.

Show comment Hide comment
@puzza007

puzza007 Jun 22, 2014

Do we need to ensure backward compatibility?

puzza007 commented Jun 22, 2014

Do we need to ensure backward compatibility?
@seth

This comment has been minimized.

Show comment Hide comment
@seth

seth Jun 22, 2014

I think the thing is:

$PROJ/include/*.hrl is assumed to be part of a project's public API by
manny people. It will help those folks if we treat it the same as we'd
treat other public facing parts of the system such as the command line
interface for this project.

Breaking back compat is sometimes the right thing to do. The project
will benefit from setting out a clear plan about how/why/when in the
hopes of preventing people from being surprised by the breakage. Two
things that will have a huge impact on how such changes are received
are: even just the impression that it was announced ahead of time, and
clear instructions on how to work-around and get unbroken.

Searching github for a list of people to reach out to -- not even
suggesting asking for permission -- just hey, this is about to change
and break you, look here for details on how to get working again (a
critical part) seems like a great idea.

And of course part of communicating breaking changes is through a
major version bump.

seth commented Jun 22, 2014

I think the thing is:

$PROJ/include/*.hrl is assumed to be part of a project's public API by
manny people. It will help those folks if we treat it the same as we'd
treat other public facing parts of the system such as the command line
interface for this project.

Breaking back compat is sometimes the right thing to do. The project
will benefit from setting out a clear plan about how/why/when in the
hopes of preventing people from being surprised by the breakage. Two
things that will have a huge impact on how such changes are received
are: even just the impression that it was announced ahead of time, and
clear instructions on how to work-around and get unbroken.

Searching github for a list of people to reach out to -- not even
suggesting asking for permission -- just hey, this is about to change
and break you, look here for details on how to get working again (a
critical part) seems like a great idea.

And of course part of communicating breaking changes is through a
major version bump.
@AeroNotix

This comment has been minimized.

Show comment Hide comment
@AeroNotix

AeroNotix Jun 22, 2014

Projects which depend on a specific version of rebar can continue doing so, no? Breaking changes are a clear delineation point to mark a different direction for the project.

AeroNotix commented Jun 22, 2014

Projects which depend on a specific version of rebar can continue doing so, no? Breaking changes are a clear delineation point to mark a different direction for the project.
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd Jun 23, 2014

Contributor

If we break the API, we should simply go for rebar 3.0 and that's it. We will have to use proper versioning so people can expect to deal with it, and that may require us to have a proper roadmap at some point? Maybe there's one already and I'm not aware of it.

I'll be regenerating rebar 2.5 with a reverted copy of this today.

Contributor

ferd commented Jun 23, 2014

If we break the API, we should simply go for rebar 3.0 and that's it. We will have to use proper versioning so people can expect to deal with it, and that may require us to have a proper roadmap at some point? Maybe there's one already and I'm not aware of it.

I'll be regenerating rebar 2.5 with a reverted copy of this today.
@tuncer

This comment has been minimized.

Show comment Hide comment
@tuncer

tuncer Jun 23, 2014

Contributor

@ferd wrote:

that may require us to have a proper roadmap at some point? Maybe there's one already and I'm not aware of it.

There's no official roadmap besides TODO tickets, and it would be useful to tag them accordingly.

Contributor

tuncer commented Jun 23, 2014

@ferd wrote:

that may require us to have a proper roadmap at some point? Maybe there's one already and I'm not aware of it.

There's no official roadmap besides TODO tickets, and it would be useful to tag them accordingly.
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd Jun 23, 2014

Contributor

@jaredmorrow is it possible to get a few more tags for issues and pull reqs? TODO / RFC / Minor / Major or something similar to track future road-mapping?

Contributor

ferd commented Jun 23, 2014

@jaredmorrow is it possible to get a few more tags for issues and pull reqs? TODO / RFC / Minor / Major or something similar to track future road-mapping?
@jaredmorrow

This comment has been minimized.

Show comment Hide comment
@jaredmorrow

jaredmorrow Jun 23, 2014

Contributor

Sure, will do that today.

On Mon, Jun 23, 2014 at 7:13 AM, Fred Hebert notifications@github.com
wrote:

@jaredmorrow https://github.com/jaredmorrow is it possible to get a few
more tags for issues and pull reqs? TODO / RFC / Minor / Major or something
similar to track future road-mapping?

—
Reply to this email directly or view it on GitHub
#281 (comment).

Contributor

jaredmorrow commented Jun 23, 2014

Sure, will do that today.

On Mon, Jun 23, 2014 at 7:13 AM, Fred Hebert notifications@github.com
wrote:

@jaredmorrow https://github.com/jaredmorrow is it possible to get a few
more tags for issues and pull reqs? TODO / RFC / Minor / Major or something
similar to track future road-mapping?

—
Reply to this email directly or view it on GitHub
#281 (comment).

ferd added a commit that referenced this pull request Jun 23, 2014

@ferd
Revert "Merge pull request #281 from tuncer/rebar-h-internal" 
This reverts commit 07e2232, reversing
changes made to 37cf470.
0b1e6b3
@ferd

This comment has been minimized.

Show comment Hide comment
@ferd

ferd Jun 23, 2014

Contributor

https://github.com/rebar/rebar/releases/tag/2.5.0 Here's the new release. Sorry for the inconvenience.

Contributor

ferd commented Jun 23, 2014

https://github.com/rebar/rebar/releases/tag/2.5.0 Here's the new release. Sorry for the inconvenience.
@jaredmorrow

This comment has been minimized.

Show comment Hide comment
@jaredmorrow

jaredmorrow Jun 23, 2014

Contributor

I created "Major", "Minor", "RFC" we already have "enhancement" so that
can act as the TODO.

On Mon, Jun 23, 2014 at 12:54 PM, Fred Hebert notifications@github.com
wrote:

https://github.com/rebar/rebar/releases/tag/2.5.0 Here's the new release.
Sorry for the inconvenience.

—
Reply to this email directly or view it on GitHub
#281 (comment).

Contributor

jaredmorrow commented Jun 23, 2014

I created "Major", "Minor", "RFC" we already have "enhancement" so that
can act as the TODO.

On Mon, Jun 23, 2014 at 12:54 PM, Fred Hebert notifications@github.com
wrote:

https://github.com/rebar/rebar/releases/tag/2.5.0 Here's the new release.
Sorry for the inconvenience.

—
Reply to this email directly or view it on GitHub
#281 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment