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

Remove *.opam.template #1753

Closed
wants to merge 2 commits into from
Closed

Remove *.opam.template #1753

wants to merge 2 commits into from

Conversation

smorimoto
Copy link
Member

No description provided.

Signed-off-by: Sora Morimoto <[email protected]>
@hhugo
Copy link
Member

hhugo commented Dec 2, 2024

The CI doesn't like this change.
Could you try to motivate changes in the description whenever you open PRs ?

@smorimoto smorimoto force-pushed the cleanup-opam-templates branch 4 times, most recently from ac99e8f to 88e32d4 Compare December 3, 2024 05:27
Signed-off-by: Sora Morimoto <[email protected]>
@smorimoto smorimoto force-pushed the cleanup-opam-templates branch from 88e32d4 to ed805f7 Compare December 3, 2024 05:29

runs-on: ${{ matrix.os }}

steps:
- name: Set git to use LF
if: matrix.ocaml-compiler < 5.2
if: matrix.os == 'windows-latest'
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. ocaml-compiler < 5.2 was on purpose.

dune-cache: true
opam-pin: false

- run: opam install conf-pkg-config
Copy link
Member

Choose a reason for hiding this comment

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

We need this, this is a temporary fix for dra27/mingw-w64-shims#1

- name: build doc
if: ${{ !matrix.skip-doc && github.event_name == 'push' && github.ref_name == 'master'}}
- name: Build doc
if: matrix.os == 'ubuntu-latest' && matrix.ocaml-compiler == '5.2' && github.event_name == 'push' && github.ref_name == 'master'
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to only bump version once when it come to change the version we generate the doc with.

- "5.0"
- "5.1"
skip-test:
Copy link
Member

Choose a reason for hiding this comment

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

I find the previous version much easier to understand.

Copy link
Member

Choose a reason for hiding this comment

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

This PR kind of inline the logic previously encoded by skip-* parameters. We know have to go over all step understand what's done for what version. I think this is going in the wrong direction.

@hhugo
Copy link
Member

hhugo commented Dec 3, 2024

Can you please explain why you want to remove opam.template and try to keep the diff minimal in the CI

@smorimoto
Copy link
Member Author

I was going to remove the excess as much as possible, but as you said, it was not as elegant as I expected.

@smorimoto smorimoto closed this Dec 5, 2024
@smorimoto smorimoto deleted the cleanup-opam-templates branch December 5, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants