Deprecate `GHC.Exts.sortWith` in favor of `Data.List.sortOn` (#12044)
Needs RevisionPublic

Authored by cblp on Oct 12 2016, 10:32 AM.

Details

Reviewers
hvr
bgamari
thomie
austin
Trac Issues
#12044
Summary

Deprecate GHC.Exts.sortWith in favor of Data.List.sortOn
Replace GHC.Exts.sortWith with Data.List.sortOn everywhere

Test Plan

standard

cblp updated this revision to Diff 8971.Oct 12 2016, 10:32 AM
cblp retitled this revision from to Deprecate `GHC.Exts.sortWith` in favor of `Data.List.sortOn` (#12044).
cblp updated this object.
cblp edited the test plan for this revision. (Show Details)
cblp added a reviewer: thomie.
cblp updated the Trac tickets for this revision.
cblp added a comment.Oct 12 2016, 10:49 AM

What's wrong with Linux build?

bgamari edited edge metadata.Oct 12 2016, 11:10 AM

@cblp, have you added an SSH public key to your Phabricator account? Harbormaster now requires that the patch be uploaded to a "staging area" repository, which requires a public key.

If you do not have a public key yet please add one and upload this Diff again with arc diff --update D2587. Let me know if you run into trouble. Thanks!

cblp added a comment.EditedOct 12 2016, 11:23 AM

@cblp, have you added an SSH public key to your Phabricator account?

Sure!

Harbormaster now requires that the patch be uploaded to a "staging area" repository, which requires a public key.

Yes, there was some errors in arc diff about staging failed caused by insufficient permissions, but the patch was uploaded to Phab succefully. Do I need to upload explicitly the patch to another part of Phab, which can't take it from this part of Phab?

If you do not have a public key yet please add one and upload this Diff again with arc diff --update D2587. Let me know if you run into trouble. Thanks!

cblp updated this revision to Diff 8973.Oct 12 2016, 11:45 AM
cblp edited edge metadata.

Replace GHC.Exts.sortWith with Data.List.sortOn everywhere (Trac #12044)

Summary:
Deprecate GHC.Exts.sortWith in favor of Data.List.sortOn
Replace GHC.Exts.sortWith with Data.List.sortOn everywhere

Test Plan: standard

Reviewers: austin, thomie, hvr, bgamari

Differential Revision: https://phabricator.haskell.org/D2587

GHC Trac Issues: Trac #12044

cblp added a comment.Oct 12 2016, 11:51 AM

When I run arc diff, it open an editor with empty body (and some comments in footer). If I close the editor, arc says that I aborted the workflow. What?

$ arc diff --update D2587
 Select a Default Commit Range 

You're running a command which operates on a range of revisions (usually,
from some revision to HEAD) but have not specified the revision that should
determine the start of the range.

Previously, arc assumed you meant 'HEAD^' when you did not specify a start
revision, but this behavior does not make much sense in most workflows
outside of Facebook's historic git-svn workflow.

arc no longer assumes 'HEAD^'. You must specify a relative commit explicitly
when you invoke a command (e.g., `arc diff HEAD^`, not just `arc diff`) or
select a default for this working copy.

In most cases, the best default is 'origin/master'. You can also select
'HEAD^' to preserve the old behavior, or some other remote or branch. But you
almost certainly want to select 'origin/master'.

(Technically: the merge-base of the selected revision and HEAD is used to
determine the start of the commit range.)

    What default do you want to use? [origin/master] 
Usage Exception: User aborted the workflow.

When I copy text from the last commit to the empty editor window, arc goes ahead:

$ arc diff
Linting...
 LINT OKAY  No lint problems.
Running unit tests...
No unit test engine is configured for this project.
 PUSH STAGING  Pushing changes to staging area...
ssh: connect to host phabricator-origin.haskell.org port 2222: Network is unreachable
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
 STAGING FAILED  Unable to push changes to the staging area.
Updated an existing Differential revision:
        Revision URI: https://phabricator.haskell.org/D2587

Included changes:
  M       compiler/deSugar/DsUtils.hs
  M       compiler/rename/RnEnv.hs
  M       compiler/rename/RnNames.hs
  M       compiler/typecheck/FamInst.hs
  M       compiler/typecheck/Inst.hs
  M       compiler/utils/Util.hs
  M       docs/users_guide/glasgow_exts.rst
  M       libraries/base/Data/Ord.hs
  M       libraries/base/GHC/Exts.hs
  M       libraries/base/changelog.md
  M       testsuite/tests/deSugar/should_run/dsrun021.hs
  M       testsuite/tests/deSugar/should_run/dsrun022.hs
  M       testsuite/tests/deSugar/should_run/mc01.hs
  M       testsuite/tests/deSugar/should_run/mc02.hs
  M       testsuite/tests/ghc-api/annotations/Test10312.hs
  M       testsuite/tests/parser/should_compile/mc16.hs
  M       testsuite/tests/parser/should_compile/read062.hs
  M       testsuite/tests/parser/should_fail/readFail042.hs
In D2587#75309, @cblp wrote:

When I run arc diff, it open an editor with empty body (and some comments in footer). If I close the editor, arc says that I aborted the workflow. What?

At this point it's asking you to summarize what has changed since the last revision of the differential that you pushed.

When I copy text from the last commit to the empty editor window, arc goes ahead:

$ arc diff
Linting...
 LINT OKAY  No lint problems.
Running unit tests...
No unit test engine is configured for this project.
 PUSH STAGING  Pushing changes to staging area...
ssh: connect to host phabricator-origin.haskell.org port 2222: Network is unreachable
fatal: Could not read from remote repository.

Wow, this is quite strange. Is there possibly something broken with your network configuration? What does ping phabricator-origin.haskell.org say?

cblp updated this revision to Diff 8977.Oct 13 2016, 3:33 AM
cblp edited edge metadata.

Summary:
Deprecate GHC.Exts.sortWith in favor of Data.List.sortOn
Replace GHC.Exts.sortWith with Data.List.sortOn everywhere

Test Plan: standard

Reviewers: austin, thomie, hvr, bgamari

Differential Revision: https://phabricator.haskell.org/D2587

GHC Trac Issues: Trac #12044

cblp added a comment.EditedOct 13 2016, 3:38 AM
In D2587#75309, @cblp wrote:

ssh: connect to host phabricator-origin.haskell.org port 2222: Network is unreachable

Wow, this is quite strange. Is there possibly something broken with your network configuration? What does ping phabricator-origin.haskell.org say?

Oh, my corporate security allows http[s] only. I can't locate "phabricator-origin.haskell.org" url to try to change it to https scheme.

Can't we proceed without "staging"?

bgamari updated this revision to Diff 8980.Oct 13 2016, 8:30 AM
bgamari edited edge metadata.

Push to staging area

bgamari accepted this revision.Oct 13 2016, 8:39 AM
bgamari edited edge metadata.
In D2587#75340, @cblp wrote:

Oh, my corporate security allows http[s] only. I can't locate "phabricator-origin.haskell.org" url to try to change it to https scheme.

Can't we proceed without "staging"?

Oh dear; I'm afraid not (at least not while retaining CI testing). That being said, it seems I can push the Diff to the staging area on your behalf (which I just did). This seems like the best solution for the time being (although I mentioned the fact that the staging area is problematic to the Phacility folks).

Otherwise, this patch looks pretty reasonable to me; if the CI builds are green then this looks good. Thanks for your patience @cblp!

This revision is now accepted and ready to land.Oct 13 2016, 8:39 AM
cblp added a comment.Oct 13 2016, 8:46 AM
In D2587#75340, @cblp wrote:

Can't we proceed without "staging"?

Oh dear; I'm afraid not (at least not while retaining CI testing). That being said, it seems I can push the Diff to the staging area on your behalf (which I just did). This seems like the best solution for the time being (although I mentioned the fact that the staging area is problematic to the Phacility folks).

I still can't understand why Phabricator user has to push their patch to two different Phabricator areas. Why Phabricator can't just take a patch from itself?

In D2587#75370, @cblp wrote:
In D2587#75340, @cblp wrote:

Can't we proceed without "staging"?

Oh dear; I'm afraid not (at least not while retaining CI testing). That being said, it seems I can push the Diff to the staging area on your behalf (which I just did). This seems like the best solution for the time being (although I mentioned the fact that the staging area is problematic to the Phacility folks).

I still can't understand why Phabricator user has to push their patch to two different Phabricator areas. Why Phabricator can't just take a patch from itself?

I'll admit I'm not the biggest fan of Phabricator's approach here; the issue is that without a staging area the only thing that Phabricator pushes is a patch; unfortunately this ends up being extremely fragile especially when the base commit against which the patch was generated is not available (as is the case with series of dependent patches).

Of course, your issue would be likely be mitigated if Harbormaster would first try to checkout the tree from the staging area and, if that fails, attempt to apply the Diff's patch. Unfortunately this isn't implemented.

cblp added a comment.Oct 13 2016, 9:00 AM

if Harbormaster would first try to checkout the tree from the staging area and, if that fails, attempt to apply the Diff's patch.

In that case (malevolent | erroneous) user still can push different versions to review and testing.

In that case (malevolent | erroneous) user still can push different versions to review and testing.

That is true.

bgamari requested changes to this revision.Oct 14 2016, 11:59 AM
bgamari edited edge metadata.

Thanks @cblp!

Looking at this a bit more closely, I don't think we want to do a blanket replacement of sortWith with sortOn. Operationally the two functions have much different behaviors, as I've described in Trac #12044. Let's pick up the discussion there until it is clear what the goal is here.

This revision now requires changes to proceed.Oct 14 2016, 11:59 AM
austin resigned from this revision.Nov 9 2017, 9:44 AM