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

Deploy with OCaml 5.2 on next #840

Merged
merged 0 commits into from
Sep 4, 2024
Merged

Deploy with OCaml 5.2 on next #840

merged 0 commits into from
Sep 4, 2024

Conversation

Firobe
Copy link
Member

@Firobe Firobe commented Aug 23, 2024

This is exactly #838 containing the work of @shym but tailored to be deployed on branch next. Its general purpose is to deploy a version of mirage-www built with ocaml-solo5 for 5.2 to benchmark its resource usage.

This PR:

  • drops the Github Action CI config from Support build with Mirage 4.5.1 and OCaml 5.2 #838
  • and instead modifies the Dockerfile used by the mirage deployer:
    • bumps the OCaml version
    • pins @shym's version of ocaml-solo5
    • hardcodes the EXTRA_FLAGS passed by the mirage deployer to remove --metrics, since mirage-memtrace isn't readily available for OCaml 5.2

That last modification is clearly a hack and should be addressed by either somehow porting mirage-memtrace to 5.2 or conditionally passing the right EXTRA_FLAGS in the mirage deployer, but this should be enough to do some benchmarks on the next branch.

This otherwise syncs next with master (dropping commits from https://github.com/mirage/mirage-www/pull/791/commits which have remained there in limbo for quite a while). Hence this will need to be force-pushed

@Firobe Firobe changed the title Ocaml52 next Deploy with OCaml 5.2 on next Aug 23, 2024
@Firobe Firobe marked this pull request as ready for review August 23, 2024 20:23
Copy link

@shym shym left a comment

Choose a reason for hiding this comment

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

Apart from a small comment I wrote on the corresponding line, this looks good to me!

Dockerfile Outdated
Comment on lines 12 to 13
ARG EXTRA_FLAGS=
RUN opam exec -- mirage configure -f mirage/config.ml -t $TARGET $EXTRA_FLAGS
ARG EXTRA_FLAGS_NO_METRICS="--tls=true --separate-networks"
Copy link

Choose a reason for hiding this comment

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

I wondered why not use EXTRA_FLAGS instead of creating the EXTRA_FLAGS_NO_METRICS.
Or, taken the other way around, if keeping EXTRA_FLAGS separate is useful, shouldn’t it still appear in the configuration command lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, the CI system always passes --metrics through EXTRA_FLAGS when building mirage-www (and I don't have control over that CI system). And I couldn't find a reasonable way to do something like EXTRA_FLAGS_NO_METRICS = $EXTRA_FLAGS without --metrics.

Another way would be for this branch to still accept --metrics but disable the underlying behavior (and not pull the dependency).

I went with the hacky solution (to hardcode what the CI passes without --metrics) to modify the code as little as possible so this experiment is representative. Then the correct solution will be to change the CI system to pass the correct flags, once we know we want to do this on main.

@samoht samoht merged commit d6c251b into mirage:next Sep 4, 2024
1 of 2 checks passed
@samoht
Copy link
Member

samoht commented Sep 4, 2024

I've force-pushed this to the next branch. Let's see if the world explodes...

@hannesm
Copy link
Member

hannesm commented Sep 11, 2024

Is there a plan / story what to do for changes to the main branch? should they be merged into "next"? Is there automation for doing that? I'm asking since it would be great to integrate #841 into the next branch as well.

@talex5
Copy link
Contributor

talex5 commented Sep 11, 2024

I don't know what the protocol is for this repo, but I'd guess that next is just used to test things, and that commits that pass the test then get merged to master to go on the main site?

But it looks like this PR failed to deploy (https://deploy.mirage.io/job/2024-09-11/142722-mirage-deploy-84f10c):

2024-09-11 14:27.24: Exec: "ssh" "[email protected]" "mirage-redeploy" "next"
albatross-client: specified bridge(s) does not match with the manifest

@Firobe
Copy link
Member Author

Firobe commented Sep 11, 2024

This is not documented for next so I'm just assuming, but my understanding is the branch exists to test changes that are not trivial (this PR is an example). So thus far most PRs have been targeting master (and I think should continue to), except when it's useful to see the results first.

Right now I would advise against putting anything else in next, since the tests for this PR involve invoking www and next with different arguments and different versions of solo5 (hence the failure above), and split traffic between the two.

Once this experiment is over we could think about a more formal process for next

@Firobe
Copy link
Member Author

Firobe commented Sep 11, 2024

We can integrate the changes of #841 to next though, to keep both in sync since traffic is split between the two. Could you open a PR?

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.

5 participants