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

Support OS400 system #139

Merged
merged 3 commits into from
Sep 17, 2022
Merged

Support OS400 system #139

merged 3 commits into from
Sep 17, 2022

Conversation

zheddie
Copy link
Contributor

@zheddie zheddie commented Aug 31, 2022

This PR is for OS400 system support. Can anybody kindly help to review and merge it? thanks.
"configure.py" is deprecated in ninja-build project, and it does not support OS400 system.

@zheddie
Copy link
Contributor Author

zheddie commented Sep 5, 2022

@mayeut , Hi , I am new to this project. Can you help review this PR ? Or help to find a correct reviewer for it ? Thanks.

@henryiii
Copy link
Contributor

henryiii commented Sep 5, 2022

Can we avoid configure.py on other systems too?

@zheddie
Copy link
Contributor Author

zheddie commented Sep 8, 2022

Yes, In theory, it should work on other platform as well. But I do not want to break other platforms with my PR. :-). If they wish, they can also follow this way.

Or , do you want me to make it done as well for all platforms? thanks.

@zheddie
Copy link
Contributor Author

zheddie commented Sep 8, 2022

@henryiii

@henryiii
Copy link
Contributor

henryiii commented Sep 8, 2022

I was waiting to see if @mayeut had any comment, since he's been involved in helping with some of these sorts of systems & Ninja.

I'd say yes, because then it's much easier to test and see if the new configuration system is working. I don't have an OS400 system to see if the new code works.

@henryiii
Copy link
Contributor

henryiii commented Sep 8, 2022

(but don't destroy your history, in case it needs revision)

@zheddie
Copy link
Contributor Author

zheddie commented Sep 13, 2022

@mayeut , any comments ? thanks.

@zheddie
Copy link
Contributor Author

zheddie commented Sep 15, 2022

Anyway, I am following the @henryiii 's suggestion to trying using the cmake on all platforms instead of IBM i only. Wish this would pass all the integration test on your available systems. I have verified on IBM i platform. thanks.

Copy link
Contributor

@henryiii henryiii left a comment

Choose a reason for hiding this comment

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

LGTM. Will wait a bit for @mayeut to have a chance to approve, then will merge (remind me if I forget).

Copy link
Contributor

@mayeut mayeut left a comment

Choose a reason for hiding this comment

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

Sorry for jumping in so late in the process.
LGTM, I'm just a bit scared of changing this because we don't have that much tests but I'll add some more in another PR.

@zheddie
Copy link
Contributor Author

zheddie commented Sep 19, 2022

Thanks for your help guys. @mayeut , can I know when can we have a release with this code available? I noticed that seems we didn't release anything in the whole of 2022? :-(.

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.

4 participants