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.

Sign up for GitHub

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

Jump to bottom

Move the PPX to a separate package (?) #338

Closed
aantron opened this issue Apr 12, 2017 · 2 comments · Fixed by #515
Closed

Move the PPX to a separate package (?) #338

aantron opened this issue Apr 12, 2017 · 2 comments · Fixed by #515
Labels
breaking
Milestone
4.0.0

Comments

@aantron
Copy link
Collaborator

aantron commented Apr 12, 2017 •
edited
Loading

I'm not sure if it ought to also be ported to ocaml-migrate-parsetree. If so, then port it.

IMO the PPX syntax is much easier to teach than >|= fun blah -> everywhere, so I've been writing new docs in terms of it. However, we can just tell newcomers to do opam install lwt lwt_ppx. Projects that avoid the PPX for whatever reason can do opam install lwt and not have to wait for ports of ppx_tools/ocaml-migrate-parsetree to new versions of OCaml.

On the other hand, maybe we should just make an effort to make sure that those ports don't take a long time. Also, ppx_tools and ocaml-migrate-parsetree are such basic packages, that installing several OPAM packages is likely to pull them in whether they are dependencies of the base lwt or not.

Please let me know any thoughts on this.

Prompted by @avsm in ocaml/opam-repository#8929 (comment).

EDIT: and there is a previous similar (but not identical) request in #266.
@Drup
Copy link
Member

Drup commented Apr 12, 2017

Splitting the ppx inside its own package and porting it to ocaml-migrate-parsetree are two completely independent tasks (and would not be done for the same reasons).

I have no opinion on the first. Doing the second while we have oasis needs far too many hacks, and it would be easier to just port lwt to something else.

@avsm
Copy link
Collaborator

avsm commented Apr 13, 2017

Agree with @Drup -- just moving the package out would be a good first start, since a significant number of packages do not use the ppx extension at all, and it's a pity when the core Lwt package fails to build when ppx is not available.

My request is specifically motivated by the desire to do more CI testing with development branches of OCaml, and this is the most common reason why ppx is unavailable (due to an AST change). Lwt blocks a very large number of packages from building, but a significant percentage of those do not have a hard ppx dependency.

@aantron aantron added this to the 4.0.0 milestone Apr 13, 2017
@aantron aantron modified the milestones: 3.1.0, 4.0.0 May 12, 2017
@aantron aantron added the medium label May 20, 2017
This was referenced May 22, 2017
Closed
Closed
aantron pushed a commit that referenced this issue Jun 8, 2017
@andrewray @aantron
Port to jbuilder
2163fb1 
This also ports the PPX to ocaml-migrate-parsetree.

The work in this commit, and the preceding one, was done by Andrew Ray
(@andrewray) in #374. These two commits are just a more blame-friendly
version; the detailed history is visible #374.

Some number of follow-on commits will probably be needed to fully
stabilize and finalize the port. Also, this commit breaks some packages
that depend on Lwt incorrectly. We will probably take steps to break
them more softly than is being done by this commit alone.

Builds on 4.05 are temporarily disabled, due to jbuilder not working
with the 4.05 betas. We will address this by building on trunk later.

This port disables coverage via Bisect completely (#319). We will
restore that in follow-on work; in the meantime, we expect few enough
edits to the test suite to be made that we can run the test suite with
coverage in a separate branch that does not yet have this commit, and
cherry-pick commits over if any development of the test suite occurs.

The build system produced by this commit is a bit of a chimera: it
adopts jbuilder without adopting modern packaging best practices. We
need to do the latter slowly, to avoid breaking projects that depend on
Lwt without proper warning (#293). Work along this direction had already
started (#301, #338, #370), and this jbuilder port will make it easier
to continue.

The port also pre-emptively solves a lot of other issues with the build
system. See the pull request for any issue links, to be added over the
coming days.

Maintainer's note: squash and summary done by Anton Bachin (@aantron).
aantron pushed a commit that referenced this issue Jun 8, 2017
@andrewray @aantron
Port to jbuilder
a7b537c 
This also ports the PPX to ocaml-migrate-parsetree.

The work in this commit, and the preceding one, was done by Andrew Ray
(@andrewray) in #374. These two commits are just a more blame-friendly
version; the detailed history is visible #374.

Some number of follow-on commits will probably be needed to fully
stabilize and finalize the port. Also, this commit breaks some packages
that depend on Lwt incorrectly. We will probably take steps to break
them more softly than is being done by this commit alone.

Builds on 4.05 are temporarily disabled, due to jbuilder not working
with the 4.05 betas. We will address this by building on trunk later.

This port disables coverage via Bisect completely (#319). We will
restore that in follow-on work; in the meantime, we expect few enough
edits to the test suite to be made that we can run the test suite with
coverage in a separate branch that does not yet have this commit, and
cherry-pick commits over if any development of the test suite occurs.

The build system produced by this commit is a bit of a chimera: it
adopts jbuilder without adopting modern packaging best practices. We
need to do the latter slowly, to avoid breaking projects that depend on
Lwt without proper warning (#293). Work along this direction had already
started (#301, #338, #370), and this jbuilder port will make it easier
to continue.

The port also pre-emptively solves a lot of other issues with the build
system. See the pull request for any issue links, to be added over the
coming days.

Maintainer's note: squash and summary done by Anton Bachin (@aantron).
aantron pushed a commit that referenced this issue Jun 8, 2017
@andrewray @aantron
Port to jbuilder
4c249bd 
This also ports the PPX to ocaml-migrate-parsetree.

The work in this commit, and the preceding one, was done by Andrew Ray
(@andrewray) in #374. These two commits are just a more blame-friendly
version; the detailed history is visible #374.

Some number of follow-on commits will probably be needed to fully
stabilize and finalize the port. Also, this commit breaks some packages
that depend on Lwt incorrectly. We will probably take steps to break
them more softly than is being done by this commit alone.

Builds on 4.05 are temporarily disabled, due to jbuilder not working
with the 4.05 betas. We will address this by building on trunk later.

This port disables coverage via Bisect completely (#319). We will
restore that in follow-on work; in the meantime, we expect few enough
edits to the test suite to be made that we can run the test suite with
coverage in a separate branch that does not yet have this commit, and
cherry-pick commits over if any development of the test suite occurs.

The build system produced by this commit is a bit of a chimera: it
adopts jbuilder without adopting modern packaging best practices. We
need to do the latter slowly, to avoid breaking projects that depend on
Lwt without proper warning (#293). Work along this direction had already
started (#301, #338, #370), and this jbuilder port will make it easier
to continue.

The port also pre-emptively solves a lot of other issues with the build
system. See the pull request for any issue links, to be added over the
coming days.

Maintainer's note: squash and summary done by Anton Bachin (@aantron).
aantron pushed a commit that referenced this issue Jun 8, 2017
@andrewray @aantron
Port to jbuilder
d8b1e59 
This also ports the PPX to ocaml-migrate-parsetree.

The work in this commit, and the preceding one, was done by Andrew Ray
(@andrewray) in #374. These two commits are just a more blame-friendly
version; the detailed history is visible #374.

Some number of follow-on commits will probably be needed to fully
stabilize and finalize the port. Also, this commit breaks some packages
that depend on Lwt incorrectly. We will probably take steps to break
them more softly than is being done by this commit alone.

Builds on 4.05 are temporarily disabled, due to jbuilder not working
with the 4.05 betas. We will address this by building on trunk later.

This port disables coverage via Bisect completely (#319). We will
restore that in follow-on work; in the meantime, we expect few enough
edits to the test suite to be made that we can run the test suite with
coverage in a separate branch that does not yet have this commit, and
cherry-pick commits over if any development of the test suite occurs.

The build system produced by this commit is a bit of a chimera: it
adopts jbuilder without adopting modern packaging best practices. We
need to do the latter slowly, to avoid breaking projects that depend on
Lwt without proper warning (#293). Work along this direction had already
started (#301, #338, #370), and this jbuilder port will make it easier
to continue.

The port also pre-emptively solves a lot of other issues with the build
system. See the pull request for any issue links, to be added over the
coming days.

Maintainer's note: squash and summary done by Anton Bachin (@aantron).
@aantron aantron modified the milestones: 3.1.0, 3.2.0 Jul 18, 2017
@aantron aantron mentioned this issue Oct 13, 2017
Merged
@aantron aantron modified the milestones: 3.2.0, 4.0.0 Oct 13, 2017
@aantron aantron mentioned this issue Oct 13, 2017
Closed
aantron added a commit that referenced this issue Nov 7, 2017
@aantron
Duplicate PPX in lwt_ppx instead of aliasing it
1ac67a5 
Lwt is factoring out the PPX as package lwt_ppx. We are currently in an
intermediate state, where lwt.ppx still exists as a subpackage of lwt,
but there is also lwt_ppx.

Before this commit, lwt_ppx was an alias for lwt.ppx. An unpleasant side
effect of this was that with ocamlfind, options passed to lwt_ppx were
not forwarded to lwt.ppx.

This commit instead packages the PPX independently in both lwt.ppx and
lwt_ppx. Options passed to lwt_ppx are consumed by lwt_ppx as one would
expect.

This dual packaging causes Findlib warnings about duplicate interface
ppx_lwt.cmi in packages lwt (lwt.ppx) and lwt_ppx. This shouldn't cause
problems in practice, since we expect each separate build process to
always use exactly one of lwt.ppx and lwt_ppx. The warnings should
disppear when lwt.ppx is eliminated.

Resolves #486.
See #338, #481.
This was referenced Dec 19, 2017
Closed
Merged
aantron added a commit that referenced this issue Dec 22, 2017
@aantron
Complete factoring out the PPX
0fa599f 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
@aantron aantron mentioned this issue Dec 23, 2017
Merged
@aantron aantron mentioned this issue Jan 22, 2018
Closed
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
5c94fb6 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
77492e7 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
2d9ff99 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
e75d16c 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
73cd62e 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
aantron added a commit that referenced this issue Mar 24, 2018
@aantron
Complete factoring out the PPX
403ca58 
The PPX was already fully packaged as lwt_ppx. This commit deletes
package lwt.ppx.

Resolves #338.
@aantron aantron mentioned this issue Mar 30, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
4.0.0
Development

Successfully merging a pull request may close this issue.

[4.0.0] Complete factoring out the PPX ocsigen/lwt
3 participants
@avsm @Drup @aantron