Skip to content

Conversation

@jonludlam
Copy link
Member

No description provided.

@jonludlam jonludlam force-pushed the module-type-of-test branch 2 times, most recently from b89ea11 to 14043d8 Compare November 22, 2023 10:22
@jonludlam jonludlam added the no changelog This pull request does not need a changelog entry label Nov 22, 2023
@EmileTrotignon EmileTrotignon added this to the 2.4.0 milestone Dec 5, 2023
Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

I have some small comments but otherwise looks good.

@@ -0,0 +1,13 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this file wasn't meant to be there ? Otherwise, compile.sh and odoc.sh shouldn't be included.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After all, I find gen.sh more readable than the full test.
It could even be inlined in run.t, generating compile.sh and odoc.sh wouldn't be necessary as commands could be called directly in the loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's there in case we ever wanted to increase the size of the test, or modify the contents of the mli files. I wrote it to do this test so I thought I might as well include it :-)

@@ -0,0 +1,16 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not a portable #! line. I suggest not having one and to run this script using bash ./compile.sh.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh? I don't understand - I think this script is pretty portable. The gen.sh needs to be changed to #!/bin/bash though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The most portable is #!/usr/bin/env bash but it's not perfect. /bin/bash is definitely not portable, it doesn't work on my machine.
It's trivial to do without this line so I think we should do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't use /bin/bash, it's /bin/sh - does that not work on your machine?

@Julow Julow removed this from the 2.4.0 milestone Dec 11, 2023
@jonludlam
Copy link
Member Author

Closing in favour of #1079

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no changelog This pull request does not need a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants