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

Update the spec and roadmap and link to them from the README #51

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

tmattio
Copy link
Contributor

@tmattio tmattio commented May 30, 2022

No description provided.

Copy link
Contributor

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

It's a little bit strange for me that the specs includes a roadmap that is different from the roadmap file.

Could we maybe rename the roadmap section in the spec file to something like "Solution" or something similar

Otherwise, looks good to me!

Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Cool, thanks! My comments are mainly questions, not really suggestions.


Hence, we want to build and distribute the platform, incrementally, and slowly evolve the existing tools to work more consistently together.

We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed previously:
Copy link
Member

Choose a reason for hiding this comment

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

With "listed previously", you mean listed in the general spec, don't you?

Suggested change
We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed previously:
We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed in [doc/spec.md](https://github.com/tarides/ocaml-platform/pull/51/files#diff-e5989c8cd8b0590afd53d2aff436163341595eafdff0cb3dc420565a7236d16f):

Copy link
Contributor

Choose a reason for hiding this comment

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

Just adjusting the link

Suggested change
We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed previously:
We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed in [doc/spec.md](https://github.com/tarides/ocaml-platform/blob/main/doc/spec.mdf):

opam_detected->opam_installed
```

When we checked that opam is installed and initialized, we go on to create an opam switch to install the other Platform tools.
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually go on to create an opam switch, do we?

If I haven't missed any change, our installer currently relies on the assumption that the opam state always contains a well-defined currently active switch. So by that assumption, after installing and initing opam if needed, there will be a well-defined currently active switch and that's where the Platform tools are installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't actually go on to create an opam switch, do we?

That is right.

Moreover, the installer script installs opam, but the ocaml-platform does nothing about that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the installer is a two step process: the installer script (runs as root and install opam and ocaml-platform on the system) and ocaml-platform (starts from "init opam" in the chart).

We don't actually go on to create an opam switch, do we?

Currently we don't but I believe we should create switches. The "cargo like" experience everyone talks about is closer to having a local switch in every projects.

- Utop - OCaml toplevel
- Merlin - Syntax completion
- Odoc - Documentation generator
- Mdx - Markdown code block execution
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit inconsistent: the list here contains mdx, whereas both the implementation of the installer and the README currently don't contain mdx (See https://github.com/tarides/ocaml-platform/issues/21.). Should we make that consistent? If so, which option do you prefer? Removing mdx from here or adding it both to the implementation of the installer and to the list in the README? (as you know, I'd do the former, but I'm ok with anything) I've also asked @NathanReb, one of the mdx maintainers, to see if he thinks mdx should be in the initial state of the platform or not. I'll let you know when I have an answer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that we should be consistent, and I would go for removing mdx, but if not we should add it to the README.

| ------------ | ------------------------ | ------------------------- | -------------------------- | ------------------------- |
| Opam | ✅ | ✅ | ✅ | ✅ |
| Dune | ✅ | ❌ [^1] | ✅ | ❌ |
| Utop | ✅ | ✅ | ✅ | ❌ |
Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's interesting! utop doesn't depend on the compiler? Does it maybe depend on the compiler just to a certain extent? I would expect it to make heavy use of compiler-libs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This ✅ is very dubious, from my experience not only it depends on the compiler but also on environment variables linking to the good compiler...

Comment on lines 136 to 161
### Execution

If we keep the binaries outside of the switch:

- How do we allow the user to select which version to use?

If we copy the binaries in the switch:

- How do we follow up with the project update? (e.g. update dune version, update ocamlformat)

Solution: we make sure the tools are installed every time we execute them.

This workflow will be used by VSCode, but let's illustrate it with some command lines:

```sh
ocaml-platform opam # Uses the latest opam outside of the user's switch
ocaml-platform utop # Uses the utop installed in the switch
ocaml-platform merlin # Uses the merlin binary copied in the switch
ocaml-platform ocamlformat # Checks the project's config and install the correct version
ocaml-platform dune # Uses the merlin binary copied in the switch
```

An execution of a tool will trigger an installation confirmation if it is not installed, so this fits perfectly with VSCode's workflow.

## VSCode integration

Copy link
Member

Choose a reason for hiding this comment

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

Is proxying the Platform tools (section Execution) and integrating the installer into VSCode (section VSCode integration) already meant for Q2?

Comment on lines 151 to 155
ocaml-platform opam # Uses the latest opam outside of the user's switch
ocaml-platform utop # Uses the utop installed in the switch
ocaml-platform merlin # Uses the merlin binary copied in the switch
ocaml-platform ocamlformat # Checks the project's config and install the correct version
ocaml-platform dune # Uses the merlin binary copied in the switch
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be a couple of typos. I'm not 100% sure what you meant, but maybe something along the following lines?

Suggested change
ocaml-platform opam # Uses the latest opam outside of the user's switch
ocaml-platform utop # Uses the utop installed in the switch
ocaml-platform merlin # Uses the merlin binary copied in the switch
ocaml-platform ocamlformat # Checks the project's config and install the correct version
ocaml-platform dune # Uses the merlin binary copied in the switch
ocaml-platform opam # Uses the latest opam outside of the user's switch
ocaml-platform utop # Uses the utop installed in the switch
ocaml-platform merlin # Uses the merlin binary copied in the switch
ocaml-platform ocamlformat # Uses the ocamlformat binary that was copied into the switch taking into account the ocamlformat version in the project's config
ocaml-platform dune # Uses the dune binary copied in the switch

doc/spec.md Outdated
Owner: @samoht
Lead maintainers: @pitag-ha / @tmattio
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Lead maintainers: @pitag-ha / @tmattio
Lead maintainers: @tmattio

Since I haven't been working very much on ocaml-platform, I think it would be a bit strange if I was added as "Lead maintainers" tbh.

doc/spec.md Outdated
- Desired output: the community converges to a well-defined, well-maintained, blessed set of tools.
- Metrics:
- The number of opam projects that use older tools.
- The number of projects developed by Tarides that are Active Platform projects.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this metric important?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that is very strange!

#### Support Enveloppe
#### Support Envelope

Our goal is to release an OCaml 5.0 Platform preview before OCaml 5 itself is released. Our focus will be on ensuring that the installer is available and works on every supported system. As such, packaging the OCaml Platform for the different package managers is a stretch goal.
Copy link
Member

Choose a reason for hiding this comment

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

I'm just wondering to make sure: do I understand correctly that you're aiming to have native Windows support for all Platform tools and for ocaml-platform itself by the time OCaml 5.0 is released?

Copy link
Member

Choose a reason for hiding this comment

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

In case that yes: see my (possibly not well-founded) objection here: https://github.com/tarides/ocaml-platform/issues/15#issuecomment-1142371747

We have been exploring various ways to integrate the Platform tools, and the design of such a CLI and have decided to roll out the Platform into three phases that each solve a different aspect of the issues listed previously:

- **Release of an installer**: an installer is distributed as a pre-built binary on all supported systems. It installs the Platform tools in the current switch by either compiling them from sources in a separate switch, or by embedding pre-built versions of the tools.
- **Integrate the Platform in the installer**: instead of installing the tools, we gradually move to a fat-binary that exposes the tools CLIs using the projects libraries.
Copy link
Member

Choose a reason for hiding this comment

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

Just as a side-note which can be ignored: as you know, I'm still not really convinced by this, at least not for all tools. If you want, whenever you (or anyone else) has time for that, you can let me know what you think about this alternative suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not convinced myself and I think @Julow is not convinced either.

I wonder if it makes sense to keep it in the q2 roadmap if all of the development team do not lean toward this solution.

@tmattio tmattio force-pushed the spec-and-roadmap branch from 3f63da1 to 8a171e7 Compare June 1, 2022 10:48
@tmattio
Copy link
Contributor Author

tmattio commented Jun 1, 2022

Thanks for the feedback everyone! I removed the Q2 roadmap as it seems it grew quite outdated as we made progress on the project. The spec is higher level and it seems we're still aligned on this, so let's keep this one only. We can work on a new roadmap if needed, although I suspect that a project board with the list of issues we're planning on working on will be enough.

@tmattio tmattio merged commit 6b2a155 into main Jun 1, 2022
@tmattio tmattio deleted the spec-and-roadmap branch June 1, 2022 10:51
@Julow Julow mentioned this pull request Jun 7, 2022
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.

4 participants