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

Fix for #273 #274

Closed
wants to merge 0 commits into from
Closed

Fix for #273 #274

wants to merge 0 commits into from

Conversation

Mechazo11
Copy link
Contributor

Issue #273 stemmed from missing the #include <cstdint> in the liveness.hpp header file. This caused a build error both in Version 3.3 and 3.2. The fix is based on the suggestion by @ivanperez-keera.

@ivanperez-keera
Copy link
Collaborator

Thanks!

I know this is a small patch, but can you please sign one of the two CLAs: https://github.com/NASA-SW-VnV/ikos/tree/master/doc/contribute. Send it by email to [email protected], CC-ing me [email protected].

I hate to do this for such a small change, but I have to, especially because we are in the process of changing licenses (to one that will be more permissive).

@Mechazo11
Copy link
Contributor Author

@ivanperez-keera sure, I will do that. This is the first time I have contributed to such a major project and might not have understood how to contribute.

@ivanperez-keera
Copy link
Collaborator

No problem at all. And thanks for being accommodating! :)

@Mechazo11
Copy link
Contributor Author

@ivanperez-keera done. Let me know if there is anything else required from my end.

@ivanperez-keera
Copy link
Collaborator

I got your email. I'll review it quickly and merge this! Thanks!

@asimonov
Copy link

@Mechazo11, thanks for reporting this.

I was trying to upgrade space-ros to jazzy and discovered the same problems (space-ros/space-ros#157 (comment)).

But in case of jazzy/ubuntu24 we need to do the same change in 3 files:

	modified:   analyzer/include/ikos/analyzer/analysis/liveness.hpp
	modified:   core/include/ikos/core/domain/machine_int/operator.hpp
	modified:   core/include/ikos/core/semantic/indexable.hpp

would it be possible for you to fix the other 2 files as you aleady have PR open?

greatly appreciated!

@Mechazo11
Copy link
Contributor Author

@asimonov thanks for the suggestion.

Dr. @ivanperez-keera, I haven't done this before so sorry asking but can I modify operator.hpp and indexable.hpp without needing to sign another CLA?

@ivanperez-keera
Copy link
Collaborator

Yes, you can. You don't need to sign another CLA. You can add it to the same PR. Thanks!

@Mechazo11
Copy link
Contributor Author

@asimonov I looked into the core/include/ikos/core/domain/machine_int/operator.hpp and core/include/ikos/core/semantic/indexable.hpp files, it appears both of them already had cstdint included. However just to make sure the change takes place, I added a comment after cstdint to indicate this Issue.

I think once Dr. @ivanperez-keera merges this PR, it should allow ikos to build successfully in Ubuntu 24.04.

@ivanperez-keera
Copy link
Collaborator

ivanperez-keera commented Aug 25, 2024

@Mechazo11 . Thanks!

I don't think we need to add a comment pointing to the issue from any of the files. It's enough to just add the include where necessary (so, only analyzer/include/ikos/analyzer/analysis/liveness.hpp needs to change).

We can modify the .gitignore to include .vscode, but it's best to do that in a separate PR because that change is really not related to #273 (you just happened to run into that problem while trying to fix #273). We will not need an additional CLA for that separate PR if you send it, @Mechazo11 . Thanks!

@Mechazo11
Copy link
Contributor Author

Applied changes as suggested.

@ivanperez-keera
Copy link
Collaborator

Thanks! Will merge ASAP.

@asimonov
Copy link

@asimonov I looked into the core/include/ikos/core/domain/machine_int/operator.hpp and core/include/ikos/core/semantic/indexable.hpp files, it appears both of them already had cstdint included. However just to make sure the change takes place, I added a comment after cstdint to indicate this Issue.

I think once Dr. @ivanperez-keera merges this PR, it should allow ikos to build successfully in Ubuntu 24.04.

@Mechazo11 ok, may be it is already in latest version. in space-ros we use v3.2, I think, which probably dont have those changes.

thank you!

@asimonov
Copy link

Thanks! Will merge ASAP.

Ivan, after your merge I can try to build within space-ros on ubuntu24 and if it all works, then can you create a new tag for us to reference for jazzy upgrade of space-ros?

@ivanperez-keera
Copy link
Collaborator

ivanperez-keera commented Aug 28, 2024

The Mac build broke, so I'm checking what's going on there. It's an unrelated and documented error. I'll try to fix that one before this is merges so that all merges have completed a successful build run.

It seems to be working on Linux (based on the successful GA run).

@asimonov
Copy link

asimonov commented Sep 4, 2024

btw, this PR version works inside space-ros Earthfile build. so, would be good to have a version tag for it for Jazzy/Noble upgrade

@ivanperez-keera
Copy link
Collaborator

I'm waiting on the lawyer to give the ok (it's the last step before it can be merged; future contributions from the same author won't require this).

Wrt to the tag, I'm expecting to make a release of IKOS before the next release of Space ROS. That's the tag that should be used.

@asimonov
Copy link

I'm waiting on the lawyer to give the ok (it's the last step before it can be merged; future contributions from the same author won't require this).

Wrt to the tag, I'm expecting to make a release of IKOS before the next release of Space ROS. That's the tag that should be used.

@ivanperez-keera any news on this?

@ivanperez-keera
Copy link
Collaborator

ivanperez-keera commented Sep 19, 2024

Hi. I spoke with the lawyers several times this week (last time Wed) to process the CLA. I'm told it's in the last stage and should be approved soon. I'll ping again on Monday.

@ivanperez-keera
Copy link
Collaborator

ivanperez-keera commented Sep 19, 2024

I just received the ok. I'll make a few changes and merge. Thank you all for your patience and thanks @Mechazo11 for your contribution, and sorry that it took so long. It's just because it was the first time, but future contributions will be quicker to process.

I'll ping you when it's merged.

@Mechazo11
Copy link
Contributor Author

Dr. @ivanperez-keera no problem, I am very happy that I was able to help out.

@ivanperez-keera
Copy link
Collaborator

Your change has been merged! Thank you once again for your contribution to ikos! I hope you contribute more in the future. We could use your help :)

@Mechazo11
Copy link
Contributor Author

Dr. @ivanperez-keera it was my pleasure to help out. I may have found another fatal error in the base space-ros image. Will be opening an issue about it today.

With best regards,
@Mechazo11

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