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

removed lwt and updated pingtest #87

Merged
merged 3 commits into from
Feb 12, 2014
Merged

removed lwt and updated pingtest #87

merged 3 commits into from
Feb 12, 2014

Conversation

arjunguha
Copy link
Member

This removes openflow.lwt. There is no point maintaining it any more. I've also updated pingtest to use async instead of lwt. Pingtest now builds when the test flag is set (I removed the end_test flag). We need to always keep tests like these building.

@seliopou
Copy link
Collaborator

seliopou commented Feb 5, 2014

Can you update travis-ci files to not build lwt?

@seliopou
Copy link
Collaborator

seliopou commented Feb 5, 2014

There are still some HighLevelSwitch*.ml files in the lib directory. Should those be killed as well?

@seliopou seliopou self-assigned this Feb 5, 2014
@arjunguha
Copy link
Member Author

Pushed already. Let it rebuild.

On Wed, Feb 5, 2014 at 10:49 AM, Spiros Eliopoulos <notifications@github.com

wrote:

Can you update travis-ci files to not build lwt?


Reply to this email directly or view it on GitHubhttps://github.com//pull/87#issuecomment-34193467
.

@arjunguha
Copy link
Member Author

No. Those are used by the Async code too.

On Wed, Feb 5, 2014 at 10:49 AM, Spiros Eliopoulos <notifications@github.com

wrote:

There are still some HighLevelSwitch*.ml files in the lib directory.
Should those be killed as well?


Reply to this email directly or view it on GitHubhttps://github.com//pull/87#issuecomment-34193591
.

@adferguson
Copy link
Contributor

I know you all like deleting code, but some folks are still using Lwt. why can't you hold-off on deleting this for another month or so?

@arjunguha
Copy link
Member Author

because it would be a pain to maintain. This is for the release in a month.

On Wed, Feb 5, 2014 at 10:52 AM, Andrew Ferguson
notifications@github.comwrote:

I know you all like deleting code, but some folks are still using Lwt. why
can't you hold-off on deleting this for another month or so?


Reply to this email directly or view it on GitHubhttps://github.com//pull/87#issuecomment-34194157
.

@adferguson
Copy link
Contributor

you're already not maintaining it (it's deprecated)

I'm asking you to simply not delete it yet, so we can continue building against master.

@jnfoster
Copy link
Member

jnfoster commented Feb 5, 2014

ADF: sorry to be the bearer of bad news. But my opinion is if you're
depending on deprecated code, I think you should probably fork. Dead code
adds clutter and complexity. And we are trying to release in a small number
of weeks. Keeping this around actually will slow us down.

-N

On Wed, Feb 5, 2014 at 10:57 AM, Andrew Ferguson
notifications@github.comwrote:

you're already not maintaining it (it's deprecated)

I'm asking you to simply not delete it yet, so we can continue building
against master.

Reply to this email directly or view it on GitHubhttps://github.com//pull/87#issuecomment-34195256
.

@adferguson
Copy link
Contributor

I agree it adds clutter. I'm confused why it adds complexity when it's already in a separate module guarded by an --enable-lwt flag.

@jnfoster
Copy link
Member

jnfoster commented Feb 5, 2014

Better: let's discuss this on the call. My quick reaction is, doesn't
forking make everyone happy?

-N

On Wed, Feb 5, 2014 at 11:23 AM, Andrew Ferguson
notifications@github.comwrote:

I agree it adds clutter. I'm confused why it adds complexity when it's
already in a separate module guarded by an --enable-lwt flag.

Reply to this email directly or view it on GitHubhttps://github.com//pull/87#issuecomment-34201029
.

@adferguson
Copy link
Contributor

sure, we can discuss on the call.

forking something big like Lwt makes everyone happy only if the upstream maintainers don't want fixes from the fork, and the fork maintainers don't want fixes from upstream (unless they are prepared to do a lot more work with git).

I would prefer not to be in that camp, but if you all do, then that's your call. it happened to us before with NetCore.

@seliopou
Copy link
Collaborator

seliopou commented Feb 5, 2014

Let's make sure we're all on the same page about why this is necessary, which involves repeating already-stated facts.

As mentioned in #65, the lwt interface is deprecated. We no longer feel an obligation to maintain it, and we won't accept patches to it. Given it is no longer being maintained, the lwt sub-package should not make it into the next release and should therefore be deleted from repository before that time. The next release is planned for the end of the month.

Until then, the lwt package can stay in the repository as long as it does not hinder development. How might it hinder development? If we change APIs that the lwt sub-package depends on, then it will likely no longer build. Even worse, it may build but exhibit incorrect behavior. SDN_Types is one such API that the lwt sub-package depends on and will likely change a lot this month. At that point, it doesn't make sense to fix those changes, and it doesn't make sense to keep broken code in the repository, even if it's behind a build flag.

To your point about git. Git is a cornucopia of esoteric features. It's why we love it. One slightly esoteric feature being merge strategies. But if that's not obscure enough, merge strategies take parameters. One such parameter is ours, which will create a merge commit from two heads, but keep all the changes on your current branch, and ignore all the changes from the other. This is essentially what you want to do except on a per-file or per-directory basis after this pull request gets merged. You can do that using another esoteric git feature. Use a .gitattributes file with a custom merge driver. It's a common enough need that if you search Stack Overflow, you'll find a tutorial on how to set this up, and an already-baked script to use as a merge driver. I believe it's a one-liner.

Once that's set up, you can merge and rebase from this repo as much as you please. For non-lwt patches that you'd like to contribute, that's as simple as creating a new branch off of this repo's master, cherry-picking some commits, and issuing a pull request. More or less, the usual workflow for open-source contributions.

In sum:

  • The next release will not include the lwt sub-package
  • The next release is scheduled for the end of the month
  • The lwt sub-package will stay in the repo (i.e., this pull request will remain unmerged) until lwt causes us development problems
  • Once that happens, I've suggested a method you can use to minimize and almost eliminate git inconveniences due to that deletion
  • If you have more git questions, you can just ask me and I'll help you figure it out

@adferguson
Copy link
Contributor

SDN_Types is one such API that the lwt sub-package depends on and will likely change a lot this month.

thank you. that's all I needed to know. if you are already planning to change the abstraction layer that Lwt & Async both implement, then I totally understand.

@seliopou
Copy link
Collaborator

seliopou commented Feb 5, 2014

Ibid., p. same.

@seliopou seliopou added this to the 0.4.0 Release milestone Feb 5, 2014
@adferguson
Copy link
Contributor

sorry, I don't understand. what do you mean? (and yes, I know what those abbreviations mean on their own :-)

@seliopou
Copy link
Collaborator

seliopou commented Feb 5, 2014

We're on the same page now.

@adferguson
Copy link
Contributor

thanks. I kept trying to figure out what previous citation you wanted to reference

@seliopou seliopou mentioned this pull request Feb 6, 2014
@jnfoster
Copy link
Member

Not to beat a dead horse, but I just lost ~45 minutes each of the past two days dealing with async flags, failures in Travis, etc. This makes me sad, because I rarely get time to hack so I'd rather not be spending it fighting with configuration settings. Maybe I'm dumb or too out of date... but I also think that keeping lwt alive does add complexity and is slowing us (or at least me) down.

@adferguson
Copy link
Contributor

sorry to hear that Nate. that sort of thing really sucks. :-(

(just to be clear: I became totally fine with this going in once Spiros pointed out that y'all were planning to change the abstraction layer as part of the push to release NetKAT. LWT was going to break all on its own once that happened.)

@jnfoster
Copy link
Member

No worries. It's mostly my fault for letting my branch sit around for so many weeks.

-N

@jnfoster
Copy link
Member

Oh to be living the #postdocballerlyfe instead.

Conflicts:
	.travis.yml
	_tags
	lib/META
	setup.ml
@seliopou
Copy link
Collaborator

Now seems like a good time to do this. @jnfoster has already hit some snags, and I'd like to get #93 merged in to ease development between now and the release.

Lwt, it was nice knowing you! (It actually wasn't but we shouldn't speak ill of the dead.)

seliopou added a commit that referenced this pull request Feb 12, 2014
removed lwt and updated pingtest
@seliopou seliopou merged commit 140e058 into master Feb 12, 2014
@seliopou seliopou deleted the remove-lwt branch February 12, 2014 00:06
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.

None yet

4 participants