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

Deprecate old benchmarking functions in std.datetime #5756

Merged
merged 5 commits into from
Oct 5, 2017

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Oct 3, 2017

These were supposed to be deprecated in 2.076, and I'd thought that I'd deprecated them already, but apparently not. So, this deprecates them.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @jmdavis!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@jmdavis
Copy link
Member Author

jmdavis commented Oct 3, 2017

I just created dlang/tools#258 to deal with the failure building DustMite.

@jmdavis jmdavis force-pushed the deprecate_benchmark branch from 003078f to 9b5da5a Compare October 3, 2017 22:18
This should make it clearer how to deal with the problems created by
having deprecated symbols in std.datetime.package that conflict with the
ones in std.datetime.stopwatch.
@jmdavis
Copy link
Member Author

jmdavis commented Oct 4, 2017

@wilzbach I have no idea what this "publictests" thing is from circleci that's failing. All of the tests that use the old benchmarking functions are deprecated, and grepping Phobos confirms that nothing outside of std.datetime.package uses the old benchmarking functions. So, I have no idea where this code that's choking on deprecation errors in the circleci build is coming from.

@wilzbach
Copy link
Member

wilzbach commented Oct 4, 2017

@wilzbach I have no idea what this "publictests" thing is from circleci that's failing. All of the tests that use the old benchmarking functions are deprecated, and grepping Phobos confirms that nothing outside of std.datetime.package uses the old benchmarking functions. So, I have no idea where this code that's choking on deprecation errors in the circleci build is coming from.

It's this unittest block in L251:

    ///
    @nogc @safe unittest
    {
        StopWatch sw;
        sw.start();
        sw.stop();
        sw.reset();
        assert(sw.peek().to!("seconds", real)() == 0);
    }

I know that the struct is marked as deprecated, but the test parser doesn't check for deprecated higher in the AST tree. As this is only one UT which is affected, the quickest route would be to remove its "public" status.

@wilzbach
Copy link
Member

wilzbach commented Oct 4, 2017

As this is only one UT which is affected, the quickest route would be to remove its "public" status.

Another quick fix would be to add deprecated to this specific unittest too.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

LGTM - though now that Martin tries very hard to publish new releases every two months, 2 years amounts to 12 releases, which imho is a very long time.
How about rather than using dates, we specify releases in which it will be removed? E.g. the symbol will be removed from the documentation by 2.080 and fully removed by 2.083?

@PetarKirov
Copy link
Member

How about rather than using dates, we specify releases in which it will be removed? E.g. the symbol will be removed from the documentation by 2.080 and fully removed by 2.083?

+1

@jmdavis
Copy link
Member Author

jmdavis commented Oct 4, 2017

A two year deprecation process is what we've been doing for years now and is what we ended up with after quite a few debates on the matter. There are folks who want stuff to be deprecated and removed quickly, and there are folks who basically want stuff to never be removed. I'm really not interested in having yet another debate about how the deprecation process works. What we've been doing gives folks plenty of time to update their code while not keeping things around forever. And honestly, my experience is that opening up that debate just risks it being decided that it's okay for deprecated symbols to sit around forever, which I think would be terrible, and I count my blessings every time we avoid that when it's brought up. Walter has complained before about symbols that disappeared on him that had gone through the who two year deprecation process. That's why undead was created in the first place (which we would just be better without IMHO, since part of the whole point was to stop having to maintain the code, which means that undead breaks occasionally, and someone complains, instead of that code just being dead).

And as for using dates, that's far easier to deal with and plan for than version numbers. Yes, Martin has gone to a lot of effort to make it so that releases are fairly regular, which was definitely not the case before, but anyone can look at the dates and know what that means for how long they have, whereas a lot fewer folks are going to familiar enough with dmd's release cycle to know in what time frames a release will likely be. And even if Martin is fairly on top of things, that doesn't mean that there isn't any drift in releases from time to time, which could further throw off the target.

The deprecation process is one year as deprecated but documented, and one year as deprecated but undocumented, and I intend to keep it that way. The only difference here is that I decided to make that clearer rather than simply say that it was being removed in a year (which is when it would be removed from the docs), and that's because these symbols unfortunately create symbol conflicts, and so it needs to be clearer when that problem will be going away, whereas normally having the symbol just disappear from the docs after a year doesn't cause issues. I've also had some folks misunderstand and think that the deprecation process is one year, so maybe I need to be clearer about it when I document that something is deprecated. But the two year process is what we've been doing for years now, and while it's not perfect, it's largely worked.

@jmdavis
Copy link
Member Author

jmdavis commented Oct 4, 2017

Okay. Hopefully that commit makes circleci happy.

@wilzbach
Copy link
Member

wilzbach commented Oct 4, 2017

@CyberShadow I see that everything is nicely green here. Is there anything from the DustMite-site blocking us (the PR is still open https://github.com/CyberShadow/DustMite/pull/52/files) or can we go ahead with this?
AFAICT once this is merged you still will have two years to update DustMite (and dlang/tools).

@CyberShadow
Copy link
Member

Is there anything from the DustMite-site blocking us

Nope.

@wilzbach wilzbach merged commit 38b86e6 into dlang:master Oct 5, 2017
@MartinNowak
Copy link
Member

Hey sorry, this broke the project tester as the tools repo uses StopWatch (and -de for whatever reason).
Please check Jenkins failures before merging PRs (https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fphobos/detail/PR-5756/6/pipeline).

@wilzbach
Copy link
Member

wilzbach commented Oct 5, 2017

Hey sorry, this broke the project tester as the tools repo uses StopWatch (and -de for whatever reason).

Urgh. Sorry - any idea why it didn't appear in the list?

image

@MartinNowak
Copy link
Member

MartinNowak commented Oct 5, 2017

That's master not the PR test. Not sure why github isn't listing the result for the merge as well, usually it's listed there. But that list was only added after the merge anyhow ;).

@wilzbach
Copy link
Member

wilzbach commented Oct 5, 2017

Anyhow, either merging dlang/tools#258 or dlang/tools#259 should fix the problem...

@jmdavis jmdavis deleted the deprecate_benchmark branch May 15, 2018 10:51
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.

7 participants