Skip to content

Conversation

@JasonGross
Copy link
Contributor

Is this change expected? Is this file supposed to be checked in? What's going on here?

@mattam82
Copy link
Member

It looks like the extraction changed somehow? The .orig file doesn't need to be checked in, you're right.

@yforster
Copy link
Member

Can we let the patch command fail if the patch is not applicable? If not, it might make sense to keep the orig file and fail if it is not correct anymore

I hunted a problem for about an hour the other day where the issue was that extraction changed and the patch silently failed, resulting in a compilation error that I couldn't make sense of

@JasonGross
Copy link
Contributor Author

Does changing
https://github.com/MetaCoq/metacoq/blob/ca64d7236c22a438496d0360385f62216ef0a2ca/template-coq/update_plugin.sh#L22-L23
with

-    patch -N -p0 < extraction.patch
-    patch -N -p0 < specFloat.patch
+    patch -N -p0 < extraction.patch || exit $?
+    patch -N -p0 < specFloat.patch || exit $?

work?

@yforster
Copy link
Member

yforster commented Dec 5, 2022

Is that different to || exit? But the idea seems to be the right one to enforce a failure on a failing patch

@JasonGross
Copy link
Contributor Author

Is that different to || exit? But the idea seems to be the right one to enforce a failure on a failing patch

I think it's the same

yforster added a commit to yforster/template-coq that referenced this pull request Dec 15, 2022
@mattam82
Copy link
Member

mattam82 commented Jan 5, 2023

I guess we can just remove the .orig file now

@JasonGross JasonGross marked this pull request as draft March 2, 2023 06:06
@mattam82
Copy link
Member

Is this still relevant?

@JasonGross
Copy link
Contributor Author

The .orig file still exists, but it seems to no longer be the case that make changes it, so, idk, maybe not?

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.

3 participants