Skip to content

Add ocamlformat back to oeedger8r#2296

Merged
oe-bors[bot] merged 3 commits intomasterfrom
andschwa/ocamlformat-12
Nov 9, 2019
Merged

Add ocamlformat back to oeedger8r#2296
oe-bors[bot] merged 3 commits intomasterfrom
andschwa/ocamlformat-12

Conversation

@andyleejordan
Copy link
Copy Markdown
Member

@andyleejordan andyleejordan commented Nov 7, 2019

PR ocaml-ppx/ocamlformat#980 fixed the ocamlformat package to install on Windows, and the fix is in the latest release 0.12. This means that we can add it back, and make running ocamlformat part of the build, but it also requires auto-formatting the edger8r code again.

@andyleejordan andyleejordan added the devtools Issue is related to Open Enclave SDK tools and debugging label Nov 7, 2019
@andyleejordan andyleejordan changed the title Add ocamlformat back to the edger8r Add ocamlformat back to oeedger8r Nov 7, 2019
@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 7, 2019

bors try

oe-bors bot pushed a commit that referenced this pull request Nov 7, 2019
@oeciteam
Try #2296:
731981f
@andyleejordan andyleejordan force-pushed the andschwa/ocamlformat-12 branch 2 times, most recently from 83047c6 to 79534de Compare November 7, 2019 23:38
@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 8, 2019

@anakrish Sorry, actually can't do it as part of the CMake build because we copy files into the build tree, and it doesn't make sense to format copied files. But mostly I've been using esy directly while developing, and then CMake at the end.

@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 8, 2019

bors try

@oe-bors
Copy link
Copy Markdown

oe-bors bot commented Nov 8, 2019

try

Already running a review

@oe-bors
Copy link
Copy Markdown

oe-bors bot commented Nov 8, 2019

try

Build failed

@andyleejordan andyleejordan force-pushed the andschwa/ocamlformat-12 branch from 79534de to 654ce50 Compare November 8, 2019 19:37
@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 8, 2019

bors try

oe-bors bot pushed a commit that referenced this pull request Nov 8, 2019
@oeciteam
Try #2296:
b9e6542
@oe-bors
Copy link
Copy Markdown

oe-bors bot commented Nov 8, 2019

try

Build succeeded

@andyleejordan andyleejordan marked this pull request as ready for review November 8, 2019 20:30
@andyleejordan andyleejordan force-pushed the andschwa/ocamlformat-12 branch from 654ce50 to 1bf6832 Compare November 8, 2019 21:42
@andyleejordan
Add ocamlformat 0.12
3ec32ae 
The next latest version fixed the symlink issue in the package, which
allows this to be installed on Windows.

Note that the syntax ~0.12.0 does not work because the published package
is no longer using semver, they dropped the trailing .0 and published
with 0.12.

Signed-off-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
@andyleejordan
Run esy dune build @fmt --auto-promote with new ocamlformat
d1ea477 
This is the last time we're doing this for a while, I swear. We couldn't
stick to 0.11 because of issues installing it on Windows, and 0.12
surprisingly changes a lot more formatting than expected.

Signed-off-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
@andyleejordan
Make esy run ocamlformat as part of the build
930c129 
Signed-off-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
@andyleejordan andyleejordan force-pushed the andschwa/ocamlformat-12 branch from 1bf6832 to 930c129 Compare November 8, 2019 22:10
anakrish
anakrish reviewed Nov 8, 2019
View reviewed changes
anakrish
anakrish approved these changes Nov 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@anakrish anakrish left a comment

Choose a reason for hiding this comment

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

LGTM

@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 9, 2019

bors r+

oe-bors bot pushed a commit that referenced this pull request Nov 9, 2019
@oeciteam @andyleejordan
Merge #2296
1b50381 
2296: Add ocamlformat back to oeedger8r r=andschwa a=andschwa

PR ocaml-ppx/ocamlformat#980 fixed the ocamlformat package to install on Windows, and the fix is in the latest release 0.12. This means that we can add it back, and make running ocamlformat part of the build, but it also requires auto-formatting the edger8r code again.

Co-authored-by: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
CodeMonkeyLeet
CodeMonkeyLeet reviewed Nov 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@CodeMonkeyLeet CodeMonkeyLeet left a comment

Choose a reason for hiding this comment

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

This change seems fine but there's a lot of new packages being pulled into the repo as a result of it. Can you write up some instructions around the maintenance of the OPAM to go with it? Is it necessary for the OPAMs to be committed or is there a build time process to pull them down?

@andyleejordan
Copy link
Copy Markdown
Member Author

andyleejordan commented Nov 9, 2019

@CodeMonkeyLeet There's really no "packages" being pulled in. The esy.lock folder is like any other lockfile, just a lot more verbose 😅 it's all just metadata recording the exact state of the package dependencies as of today (which is something we desire for reproducible builds). The actual packages (their sources and resulting artifacts) are pulled in at build-time to ~/.esy, not any where in the repo. There is no OPAM maintenance to speak of, as we are not using the OCaml tools or OPAM tools directly, it's all bundled by esy.

@oe-bors
Copy link
Copy Markdown

oe-bors bot commented Nov 9, 2019

Build succeeded

@oe-bors oe-bors bot merged commit 930c129 into master Nov 9, 2019
@oe-bors oe-bors bot deleted the andschwa/ocamlformat-12 branch November 9, 2019 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anakrish anakrish anakrish approved these changes

@BRMcLaren BRMcLaren Awaiting requested review from BRMcLaren

@EmilAlexandruStoica EmilAlexandruStoica Awaiting requested review from EmilAlexandruStoica

@johnkord johnkord Awaiting requested review from johnkord

@jxyang jxyang Awaiting requested review from jxyang

@radhikaj radhikaj Awaiting requested review from radhikaj radhikaj is a code owner

+1 more reviewer

@CodeMonkeyLeet CodeMonkeyLeet CodeMonkeyLeet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@anakrish anakrish

Labels

devtools Issue is related to Open Enclave SDK tools and debugging

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andyleejordan @CodeMonkeyLeet @anakrish