Re: Action Required: Status of Harbormaster for Differentials
Hi Ben, Thanks for tackling this difficult issue -- I know you've been looking for a better solution. As I understand it, anyone will now need to add a public key to Phab in order to upload patches. This (as you admit) will increase the barriers to contributions. Are the instructions for how a new contributor should submit patches (without a public key and therefore without CI) posted very visibly somewhere? Thanks, Richard > On Aug 26, 2016, at 9:38 AM, Ben Gamari wrote: > > tl;dr. I am working on restoring Harbormaster support for Differentials. > Towards this end, GHC's Phabricator instance will soon require > that you add an SSH public key to your account in order to upload > patches with `arc diff`. > > > Hello everyone, > > As you are no doubt aware Harbormaster has not been building > Differentials for a very long time now. I am currently looking at > options for restoring this service but it appears that doing so will > require a bit of action on the part of our contributors. > > Specically, you will need to add an SSH key to Phabricator. You can do > so by navigating to the Phabricator Settings [1] page, selecting the > "Personal Account Settings" option, and selecting "SSH Public Keys" from > the list on the left. There you can click on the "SSH Key Actions" > menu on the top-right corner and click on the "Upload Public Key" > item. Here you can specify a key name and paste the key contents > (e.g. the contents of $HOME/.ssh/id_rsa.pub). > > The reason for this new requirement is that `arc diff` will soon push > submitted diffs to a "staging area", a Git repository managed by Phabricator > containing a branch for each submitted Differential. This will allow more > reliable Harbormaster builds and merges as we will no longer need to > rely on Phabricator correctly applying Differential patches (which has > historically been problematic, especially for Differentials where the > base commit is unavailable). > > If there are no objections, I would like to enable the staging area for > the rGHC repository in three days, on Monday, 29 August 2016. After > this date `arc diff` will try to push submitted differentials to the > staging area, which will require a public key. Note, however, that > pushing to the staging area can always be disabled with `arc diff`'s > `--skip-staging` flag (although we won't be able to run CI builds on > Differentials submitted in this way). > > To be clear: I would have liked to provide an option that did not > require key-based authentication, but sadly neither Phabricator nor > Gitolite provide any such option. I apologize for the added friction > that this new requirement imposes. If enough people feel strongly that > this is too onerous then I'm happy to entertain alternative solutions. > > Cheers, > > - Ben > > > [1] https://phabricator.haskell.org/settings/ > ___ > ghc-devs mailing list > ghc-devs@haskell.org > http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs ___ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Re: Last call for merge requests for 8.0.2
Facundo Domínguez writes: > Hello Ben, > > Could we have these patches added? > > http://git.haskell.org/ghc.git/commit/56f47d4a4e418235285d8b8cfe23bde6473f17fc > http://git.haskell.org/ghc.git/commit/567dbd9bcb602accf3184b83050f2982cbb7758b > > These allow reify to reach local variables when used with addModFinalizer. > Hmm, I'm not sure; #11832 is strictly speaking a feature request which we try to avoid merging in minor releases to avoid introducing bugs. Given that 8.2.1 isn't that far away (hopefully early 2017), how terrible would it be if we punted this? Cheers, - Ben signature.asc Description: PGP signature ___ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Re: Last call for merge requests for 8.0.2
Depends how you look at it I guess. It's arguably a fix to addModFinalizer,
which never really worked for the use case it was intended since it was
first introduced. This function was added by the author of
language-c-inline, who wanted things like
cos :: Double -> Double
cos x = [c| cos($x) |]
to work by having a new C wrapper function for the quasiquote be generated
and written to disk at the end of the type checking phase. Doing it at the
end means all quasiquotes in the whole module can be dumped at once.
Problem is, the type environment was not available by then, so the user was
forced to provide an explicit type annotation to help with the code
generation, as is done in inline-c:
cos :: Double -> Double
cos x = [c| double { cos($double:x) } |]
>From GHC 7.8, types for locally bound variables weren't even available at
the quasiquotation site, for reasons. But it's perfectly safe to make them
available by the time all type checking is finished.
We could wait of course, but in the meantime this is completely holding up
the "inline" features of inline-java. Because for inline-java we decided
that the (redundant) type annotations such as the above were really too
verbose in the case of Java, and would require significant hacks to the
language-java parser, so we don't support them. That's why I for one am
keen to have addModFinalizer make into a release as soon as possible.
But I completely understand your risk aversion for a point release. Here's
some data to help you evaluate the risk: the patch is specific to TH
finalizers registered using addModFinalizer, a function that is currently
used by only one package out there at the moment: expandth (based on github
code search - short of being able to code search all of Hackage directly).
--
Mathieu Boespflug
Founder at http://tweag.io.
On 30 August 2016 at 17:59, Ben Gamari wrote:
> Facundo Domínguez writes:
>
> > Hello Ben,
> >
> > Could we have these patches added?
> >
> > http://git.haskell.org/ghc.git/commit/56f47d4a4e418235285d8b8cfe23bd
> e6473f17fc
> > http://git.haskell.org/ghc.git/commit/567dbd9bcb602accf3184b83050f29
> 82cbb7758b
> >
> > These allow reify to reach local variables when used with
> addModFinalizer.
> >
> Hmm, I'm not sure; #11832 is strictly speaking a feature request which
> we try to avoid merging in minor releases to avoid introducing bugs.
> Given that 8.2.1 isn't that far away (hopefully early 2017), how
> terrible would it be if we punted this?
>
> Cheers,
>
> - Ben
>
>
> ___
> ghc-devs mailing list
> ghc-devs@haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
>
>
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Re: Action Required: Status of Harbormaster for Differentials
Richard Eisenberg writes: > Hi Ben, > > Thanks for tackling this difficult issue -- I know you've been looking > for a better solution. > > As I understand it, anyone will now need to add a public key to Phab > in order to upload patches. This (as you admit) will increase the > barriers to contributions. Are the instructions for how a new > contributor should submit patches (without a public key and therefore > without CI) posted very visibly somewhere? > I have some edits of the Phabricator page [1] in the Wiki on-going as we speak (although mpickering deserves credit for doing the original edits working in mentions of the SSH key requirement). Cheers, - Ben [1] https://ghc.haskell.org/trac/ghc/wiki/Phabricator signature.asc Description: PGP signature ___ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
RE: [commit: ghc] master: Remove unused DerivInst constructor for DerivStuff (f4384ef)
Ryan
I just wanted to thank you for all the work you are doing on 'deriving' and
generics.
It's a very useful part of GHC, but it's sufficiently complicated that it's
hard to dive in and make a quick fix. You are giving it sustained attention,
and have become very familiar with the code, which is exactly what it needs.
Thank you! It's extremely valuable work.
I am conscious that I owe you a code review on D2280, but this week is bad and
next week is worse. If I don't get to it, let's sit down at ICFP. I should
not be standing in the way. Has anyone else given it a good review?
Simon
-Original Message-
From: ghc-commits [mailto:ghc-commits-boun...@haskell.org] On Behalf Of
g...@git.haskell.org
Sent: 29 August 2016 20:29
To: ghc-comm...@haskell.org
Subject: [commit: ghc] master: Remove unused DerivInst constructor for
DerivStuff (f4384ef)
Repository : ssh://g...@git.haskell.org/ghc
On branch : master
Link :
https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fghc.haskell.org%2ftrac%2fghc%2fchangeset%2ff4384ef5b42bb64b55d6c930ed9850a021796f36%2fghc&data=01%7c01%7csimonpj%40microsoft.com%7cde5175bbe587432dbce208d3d042b188%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=l7WnAtIxsN0brRXa5FKnFrEvk0pyxm48BIqyOiiiIx4%3d
>---
commit f4384ef5b42bb64b55d6c930ed9850a021796f36
Author: Ryan Scott
Date: Mon Aug 29 15:26:53 2016 -0400
Remove unused DerivInst constructor for DerivStuff
Summary:
Back when derived `Generic` instances used to generate auxiliary datatypes,
they would also generate instances for those datatypes. Nowadays, GHC
generics
uses a `DataKinds`-based encoding that requires neither auxiliary datatypes
(corresponding to the `DerivTyCon` constructor of `DerivStuff`) nor
instances
for them (the `DerivInst` constructor of `DerivStuff`). It appears that
`DerivTyCon` constructor was removed at some point, but `DerivInst` never
was.
No `DerivInst` values are ever constructed, so we can safely remove it.
Test Plan: It builds
Reviewers: austin, hvr, bgamari
Reviewed By: bgamari
Subscribers: thomie
Differential Revision: https://phabricator.haskell.org/D2481
>---
f4384ef5b42bb64b55d6c930ed9850a021796f36
compiler/typecheck/TcDeriv.hs| 5 ++---
compiler/typecheck/TcGenDeriv.hs | 22 +-
2 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/compiler/typecheck/TcDeriv.hs b/compiler/typecheck/TcDeriv.hs
index 70eaf5c..e38cfdc 100644
--- a/compiler/typecheck/TcDeriv.hs
+++ b/compiler/typecheck/TcDeriv.hs
@@ -387,13 +387,12 @@ tcDeriving deriv_infos deriv_decls
; let (inst_infos, deriv_stuff, maybe_fvs) = unzip3 (insts1 ++ insts2)
; loc <- getSrcSpanM
-; let (binds, famInsts, extraInstances) =
-genAuxBinds loc (unionManyBags deriv_stuff)
+; let (binds, famInsts) = genAuxBinds loc (unionManyBags
+ deriv_stuff)
; dflags <- getDynFlags
; (inst_info, rn_binds, rn_dus) <-
-renameDeriv is_boot (inst_infos ++ (bagToList extraInstances))
binds
+renameDeriv is_boot inst_infos binds
; unless (isEmptyBag inst_info) $
liftIO (dumpIfSet_dyn dflags Opt_D_dump_deriv "Derived instances"
diff --git a/compiler/typecheck/TcGenDeriv.hs b/compiler/typecheck/TcGenDeriv.hs
index 2eb8c07..dce0b16 100644
--- a/compiler/typecheck/TcGenDeriv.hs
+++ b/compiler/typecheck/TcGenDeriv.hs
@@ -70,7 +70,6 @@ import Lexeme
import FastString
import Pair
import Bag
-import TcEnv (InstInfo)
import StaticFlags( opt_PprStyle_Debug )
import ListSetOps ( assocMaybe )
@@ -90,12 +89,11 @@ data AuxBindSpec
data DerivStuff -- Please add this auxiliary stuff
= DerivAuxBind AuxBindSpec
- -- Generics
+ -- Generics and DeriveAnyClass
| DerivFamInst FamInst -- New type family instances
-- New top-level auxiliary bindings
| DerivHsBind (LHsBind RdrName, LSig RdrName) -- Also used for SYB
- | DerivInst (InstInfo RdrName)-- New, auxiliary instances
{-
@@ -2346,11 +2344,11 @@ genAuxBindSpec loc (DerivMaxTag tycon)
max_tag = case (tyConDataCons tycon) of
data_cons -> toInteger ((length data_cons) - fIRST_TAG)
-type SeparateBagsDerivStuff = -- AuxBinds and SYB bindings
- ( Bag (LHsBind RdrName, LSig RdrName)
--- Extra bindings (used by Generic only)
- , Bag (FamInst) -- Extra family
instances
- , Bag (InstInfo RdrName)) -- Extra instances
+type SeparateBagsDerivStuff =
+ -- AuxBinds and SYB bindings
+ ( Bag (LHsBind RdrName, LSig RdrName)
+ -- Extra family instances
Re: Last call for merge requests for 8.0.2
"Boespflug, Mathieu" writes:
> Depends how you look at it I guess. It's arguably a fix to addModFinalizer,
> which never really worked for the use case it was intended since it was
> first introduced. This function was added by the author of
> language-c-inline, who wanted things like
>
> cos :: Double -> Double
> cos x = [c| cos($x) |]
>
> to work by having a new C wrapper function for the quasiquote be generated
> and written to disk at the end of the type checking phase. Doing it at the
> end means all quasiquotes in the whole module can be dumped at once.
> Problem is, the type environment was not available by then, so the user was
> forced to provide an explicit type annotation to help with the code
> generation, as is done in inline-c:
>
> cos :: Double -> Double
> cos x = [c| double { cos($double:x) } |]
>
> From GHC 7.8, types for locally bound variables weren't even available at
> the quasiquotation site, for reasons. But it's perfectly safe to make them
> available by the time all type checking is finished.
>
> We could wait of course, but in the meantime this is completely holding up
> the "inline" features of inline-java. Because for inline-java we decided
> that the (redundant) type annotations such as the above were really too
> verbose in the case of Java, and would require significant hacks to the
> language-java parser, so we don't support them. That's why I for one am
> keen to have addModFinalizer make into a release as soon as possible.
>
> But I completely understand your risk aversion for a point release. Here's
> some data to help you evaluate the risk: the patch is specific to TH
> finalizers registered using addModFinalizer, a function that is currently
> used by only one package out there at the moment: expandth (based on github
> code search - short of being able to code search all of Hackage directly).
>
Fair enough; this is compelling enough a reason to me. I've gone ahead
and merged it. However, would either you or Facundo be able to write a
release notes entry?
Thanks,
- Ben
signature.asc
Description: PGP signature
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
RE: Last call for merge requests for 8.0.2
If it merges ok, I’d be inclined to put this in.
·We know it’ll help someone (Mathieu and colleages).
·It (should) affect a new and as-yet little-used feature, so it’s
unlikely to harm anyone.(Of course, ANY change can have unexpected
consequences.)
·And you could regard it as a kind of bug-fix.
I think we could bend the rules here.
Simon
From: ghc-devs [mailto:ghc-devs-boun...@haskell.org] On Behalf Of Boespflug,
Mathieu
Sent: 30 August 2016 19:05
To: Ben Gamari
Cc: GHC developers
Subject: Re: Last call for merge requests for 8.0.2
Depends how you look at it I guess. It's arguably a fix to addModFinalizer,
which never really worked for the use case it was intended since it was first
introduced. This function was added by the author of language-c-inline, who
wanted things like
cos :: Double -> Double
cos x = [c| cos($x) |]
to work by having a new C wrapper function for the quasiquote be generated and
written to disk at the end of the type checking phase. Doing it at the end
means all quasiquotes in the whole module can be dumped at once. Problem is,
the type environment was not available by then, so the user was forced to
provide an explicit type annotation to help with the code generation, as is
done in inline-c:
cos :: Double -> Double
cos x = [c| double { cos($double:x) } |]
From GHC 7.8, types for locally bound variables weren't even available at the
quasiquotation site, for reasons. But it's perfectly safe to make them
available by the time all type checking is finished.
We could wait of course, but in the meantime this is completely holding up the
"inline" features of inline-java. Because for inline-java we decided that the
(redundant) type annotations such as the above were really too verbose in the
case of Java, and would require significant hacks to the language-java parser,
so we don't support them. That's why I for one am keen to have addModFinalizer
make into a release as soon as possible.
But I completely understand your risk aversion for a point release. Here's some
data to help you evaluate the risk: the patch is specific to TH finalizers
registered using addModFinalizer, a function that is currently used by only one
package out there at the moment: expandth (based on github code search - short
of being able to code search all of Hackage directly).
--
Mathieu Boespflug
Founder at
http://tweag.io.
On 30 August 2016 at 17:59, Ben Gamari
mailto:b...@well-typed.com>> wrote:
Facundo Domínguez
mailto:facundo.doming...@tweag.io>> writes:
> Hello Ben,
>
> Could we have these patches added?
>
> http://git.haskell.org/ghc.git/commit/56f47d4a4e418235285d8b8cfe23bde6473f17fc
> http://git.haskell.org/ghc.git/commit/567dbd9bcb602accf3184b83050f2982cbb7758b
>
> These allow reify to reach local variables when used with addModFinalizer.
>
Hmm, I'm not sure; #11832 is strictly speaking a feature request which
we try to avoid merging in minor releases to avoid introducing bugs.
Given that 8.2.1 isn't that far away (hopefully early 2017), how
terrible would it be if we punted this?
Cheers,
- Ben
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
___
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
hsc2hs fix
Ben et al, I'm responding to your last call for merge requests ( https://mail.haskell.org/pipermail/ghc-devs/2016-August/012693.html). I would really like to get this fix to hsc2hs in if it all possible: https://phabricator.haskell.org/D2478 Thanks, Richard. ___ ghc-devs mailing list ghc-devs@haskell.org http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
