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

test: reproduce @ocaml-index failing with package management #10986

Closed
wants to merge 3 commits into from

Conversation

liate7
Copy link
Contributor

@liate7 liate7 commented Oct 3, 2024

reproduces the build failure in #10985

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Thanks for the repro case, these are a great way to test potential bugfixes. Here's my observations on the PR.

being in its own directory.

This can't be just written to disk, I /think/ so it cat get an absolute path
in the source stanza.
Copy link
Collaborator

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 paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That paragraph only applies to the next command. Originally, I just had the build command in the test case, which didn't work, so I moved building the package file back to here.

test/blackbox-tests/test-cases/ocaml-index/gh10985/run.t Outdated Show resolved Hide resolved
The only real change between this file and pkg/libraries.t is the build command.

Signed-off-by: Andrew Patterson <[email protected]>
@liate7
Copy link
Contributor Author

liate7 commented Oct 3, 2024

I've updated the pr to put the issue in a single cram file.

> EOF

$ cat >dune <<EOF
> (dirs (:standard \ external_sources))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably avoid this by putting external_sources in one folder, the repo that uses it in another and that refer to it via (copy $PWD/../external_sources).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, this file is the same as test/blackbox-tests/test-cases/pkg/libraries.t, just with a slightly different comment and building the index instead of foo.cma. I literally started by copying over that file. I can definitely still make this change, it just seems weird to change this from the directly analogous working test.

(If I don't change it, maybe it's worth noting this in the file, instead of just in the commit description?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just made this change. If this really matters, I can also send a separate pull request to update pkg/libraries.t.

@rgrinberg
Copy link
Member

Merged in #10986. Thanks

@rgrinberg rgrinberg closed this Oct 5, 2024
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.

4 participants