-
Notifications
You must be signed in to change notification settings - Fork 55
Only copy files if they exist #479
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
base: main
Are you sure you want to change the base?
Only copy files if they exist #479
Conversation
Thanks, appreciated! In case you need further changes, can you please wait until tomorrow? We are trying to get the 9.0 preview out of the door and the CI bandwidth per project is limited and our CI takes a long time even without bandwidth limitations, so this slows us down a bit. We might also cancel your CI runs to prioritise ours - we will restart them then when we are finished. |
Yes, will wait until tomorrow. Sorry I thought I had tested the first run, but I had changed the wrong file. |
No issue at all - I just wanted to explain that we might do unexpected things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In summary, we should review the way the icon is retrieved and make it independent of CoqIDE rather than adding logic which depends on if CoqIDE is there or not.
The change for CompCert is fine, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jim-portegies : I guess the two icon files are the same and coq-shell.ico always exists, so I think we should always use this one and remove the ifs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think the icons are in fact different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I remember that one should be a console icon with a Coq on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is more or less the same as above: instead of switching between various icons, we should just make the method to retrieve the icon independend of CoqIDE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSoegtropIMC I agree, shall I try to make an update to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. It would be nice if installers for older version still would get the old icon.
So far, building the Windows installer through the script
create_windows_installer.sh
failed when either Coqide was not installed or when compcert was not installed. This PR aims to fix that.