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(pkg): reproduce not finding the patch file (dune fmt) #10947

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

moyodiallo
Copy link
Collaborator

It reproduce #10903.

> (public_name ocamlformat))
> EOF
$ cd ..
$ tar -czf ocamlformat.tar.gz ocamlformat
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, but if you don't compress the tarball then you avoid a dependency on gzip.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wouldn't that mean ocamlformat directory could not be removed ?

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Sep 23, 2024

Choose a reason for hiding this comment

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

I don't understand. What I'm saying is, since you're using the z option in tar it will involve using gzip to pack the tarball and dune will use gzip to unpack the tarball. But given we don't really care about the size or distributing it, you can just create the tarball without compression, thus one less place where gzip is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK I get it, fair point. Now this PR is using the helpers.sh file.
I think it would be better to remove the option z inside helpers.sh file in another PR, since the all ocamlformat tests are using it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, opened #10952.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks.

@moyodiallo moyodiallo changed the title test(pkg): reproduce not finding the patch file test(pkg): reproduce not finding the patch file (dune fmt) Sep 23, 2024
@@ -0,0 +1,71 @@
This is a current bug, 'dune fmt' misses extra files from "dev-tools.locks" directory for the first run.
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 really understand the bug from this description

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll rework it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it could be more comprehensible for now.

@moyodiallo moyodiallo force-pushed the test-for-bug-extra-files branch 2 times, most recently from 05f0596 to f7e9b61 Compare September 24, 2024 16:11
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
@moyodiallo
Copy link
Collaborator Author

The suggestions are applied, need another review here.

@Leonidas-from-XIV @rgrinberg

> EOF

Make the ocamlformat opam package which uses a patch:
$ mkpkg ocamlformat "0.26.2" <<EOF
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need the solving step? Can't you write a fake lock directory for ocamlformat?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah we need it. The bug is when the solving step is done during the fisrt run of dune fmt and if you have already the lock directory it won't trigger the bug.

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.

3 participants