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

Switch to new packfile API and enable --target=pbc #76

Closed
wants to merge 3 commits into from

Conversation

gerdr
Copy link
Contributor

@gerdr gerdr commented Feb 22, 2013

Needs to be applied together with the corresponding changes to Parrot and Rakudo.

This splits the stage 'evalpmc' into two stages 'pbc' and 'init',
making --target=pbc work
This is necessary as we're doing a breaking change,
relying on a new Parrot API
@pmichaud
Copy link
Contributor

I want to modify these commits a bit before applying them.

Pm

@coke
Copy link
Contributor

coke commented Oct 8, 2013

@pmichaud - any direction you can provide on the changes you wanted to make here?

@pmichaud
Copy link
Contributor

pmichaud commented Oct 8, 2013

@coke - well, the biggest obstacle on this pull request is that it's blocking on parrot/parrot#937 . I don't know when/if that will resolve.

Beyond that, I wanted to clean up the names of some of the variables (the pull request uses "main" and $mainline in a way that is wholly inconsistent with NQP/Rakudo's uses of the term), and also do something useful with the --target=evalpmc option instead of eliminating it entirely (i.e., a softer deprecation).

But this is an issue that is truly blocked on Parrot for the moment, and has been since February.

Pm

@gerdr
Copy link
Contributor Author

gerdr commented Oct 8, 2013

This is not really blocked on Parrot. The reason why it never landed is because it's a two-way breaking change, ie the pull requests to Parrot/NQP/Rakudo would have to be applied simultaneously (which we discussed in #parrotsketch). Parrot cannot just go merge parrot/parrot#973 on its own as this would break NQP.

When I submitted the pull requests, everything was ready to go. What happened was that while I was waiting for pmichaud's modifications, jnthn refactored HLL::Compiler and bit-rot set in.

As no one besides myself seemed particularly interested in whiteknight's refactoring work or in getting rid of the 2-stage -> PIR -> PBC compilation, I moved on as well.

@pmichaud
Copy link
Contributor

pmichaud commented Oct 8, 2013

This is not really blocked on Parrot. The reason why it never
landed is because it's a two-way breaking change, ie the pull
requests to Parrot/NQP/Rakudo would have to be applied
simultaneously (which we discussed in #parrotsketch).

No -- and please don't whitewash history.

In that #parrotsketch meeting I explicitly asked for Parrot to go ahead and make the changes and that Rakudo/NQP would follow suit. From http://irclog.perlgeek.de/parrotsketch/2013-02-19#i_6475005 :

21:11 <pmichaud> also, my takeaway from the earlier discussion is
  that whenever someone wants to merge in the evalpmc changes to 
  Parrot, we'll go ahead and update Rakudo/NQP to match (which  
  will bump up the parrot revision requirement).  I may decide
  to-reimplement the nqp/rakudo patches myself, though, rather
  than adopt the pull requests.                               

Then in the issue ticket, I repeated this in April ( parrot/parrot#937 (comment) ):

The Parrot side of the patch can go in anytime after the 5.3.0
release, we'll update the NQP and Rakudo sides to match shortly
after that.                                                    

Parrot keeps claiming that they're not the blockers, but I've repeatedly asked for the changes to be added to Parrot, or for there to be an up-to-date Parrot branch with those changes, and nothing has happened on that front since February.

Pm

@gerdr
Copy link
Contributor Author

gerdr commented Oct 8, 2013

My point is, if anyone really cared about these changes, it would have been a simple thing to check out the pull request, merge in master and see if it still builds.

I just verified that it indeed does so, at least on my machine.

As far as I can tell, codewise the Parrot side is as ready as it was 8 months ago.

If all that's blocking the NQP side from going forward is a branch in the main Parrot repository, someone with a commit bit should add that ;)

@Benabik
Copy link
Contributor

Benabik commented Oct 8, 2013

I added a new-packfile-api branch to parrot/parrot which merges gerdr/parrot new-packfile-api into master.

I performed no testing, just made the branch available so others can.

@rurban
Copy link

rurban commented Oct 8, 2013

new parrot/new-packfile-api with old nqp/new-packfile-api passes all tests.
Can someone familiar with latest nqp rebase new-packfile-api onto master?

I'll merge parrot#937 new-packfile-api also then.
There are several nqp conflicts which I feel not being able to merge

@pmichaud
Copy link
Contributor

pmichaud commented Oct 8, 2013

Do not merge any packfile-related changes into nqp's master branch until after there's been at least one Parrot release containing the new changes. In fact, I suspect we need to wait until there's a "supported release" for Parrot containing the new packfile API changes.

Pm

@Benabik
Copy link
Contributor

Benabik commented Oct 8, 2013

I suspect the concept of "supported release" is historical at this point, given the current development speed and number of active developers in Parrot.

@rurban
Copy link

rurban commented Oct 8, 2013

pmichaud: I just meant an updated nqp/new-packfile-api branch would be nice.

g co new-packfile-api
g rebase master
...manually fix conflicts...
g push

:)

@coke
Copy link
Contributor

coke commented Oct 8, 2013

There is no effective difference between supported and dev releases, and
haven't been for some time.

If we need a release, the next available one should be fine.

On Tue, Oct 8, 2013 at 2:44 PM, Patrick R. Michaud <notifications@github.com

wrote:

Do not merge any packfile-related changes into nqp's master branch until
after there's been at least one Parrot release containing the new changes.
In fact, I suspect we need to wait until there's a "supported release" for
Parrot containing the new packfile API changes.

Pm


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

Will "Coke" Coleda

@pmichaud
Copy link
Contributor

pmichaud commented Oct 8, 2013

Last I heard, the Debian packagers still paid attention to Parrot's notions of "supported" and "developer" releases, and the website still does. So I think we may still need to be aware of the distinction as viewed by others, even if we no longer honor it internally.

@ghost ghost assigned pmichaud Oct 8, 2013
pmichaud added a commit that referenced this pull request Oct 9, 2013
Resolves issue #76, but requires a Parrot newer than
RELEASE_5_7_0-20-g07dfdb4.  See issue #76 for more details.
@pmichaud
Copy link
Contributor

pmichaud commented Oct 9, 2013

Tonight I've made some updates to make the migration easier. First, I added two methods to Parrot's EvalPMC (all_subs and is_initialized) that enables it to act like the PackfileView PMC, at least for what NQP and Rakudo need.

In the nqp repository I've created a pm-packfile-api branch that works with either Parrot's master branch (EvalPMC) or with Parrot's new-packfile-api branch. I've spectested both nqp and rakudo against Parrot builds of both branches.

I'm okay with merging the pm-packfile-api branch of nqp to master whenever we're comfortable with bumping PARROT_REVISION. Once pm-packfile-api is merged, it should be able to handle a Parrot switch to PackfileView without any difficulties, and we can close this issue.

I'll add the remaining capabilities (directly compiling to .pbc) as part of a separate set of patches, or under a separate issue ticket. We should probably wait for Parrot to officially switch to PackfileView to do this.

Pm

pmichaud added a commit that referenced this pull request Oct 16, 2013
Resolves issue #76, but requires a Parrot newer than
RELEASE_5_7_0-20-g07dfdb4.  See issue #76 for more details.
pmichaud added a commit that referenced this pull request Oct 16, 2013
@pmichaud pmichaud mentioned this pull request Oct 16, 2013
@pmichaud
Copy link
Contributor

pm-packfile-api branch merged as of dc9d7ff . NQP now uses the PackfileView API for compilation (EvalPMC supports enough of the API to enable compilation to work as it has before).

The issue of adding a --target=pbc option is now issue #139, which I'll work on when the PackfileView API gets merged into Parrot master.

Closing issue, thanks to everyone that contributed.

Pm

@pmichaud pmichaud closed this Oct 16, 2013
pmichaud added a commit that referenced this pull request Oct 29, 2013
Resolves issue #76, but requires a Parrot newer than
RELEASE_5_7_0-20-g07dfdb4.  See issue #76 for more details.
pmichaud added a commit that referenced this pull request Oct 29, 2013
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

5 participants