Skip to content

Support OS400 system#139

Merged
mayeut merged 3 commits into
scikit-build:masterfrom
zheddie:supportos400
Sep 17, 2022
Merged

Support OS400 system#139
mayeut merged 3 commits into
scikit-build:masterfrom
zheddie:supportos400

Conversation

@zheddie
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

henryiii commented Sep 5, 2022

Can we avoid configure.py on other systems too?

@zheddie
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

zheddie commented Sep 8, 2022

@henryiii

@henryiii
Copy link
Copy Markdown
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
Copy Markdown
Contributor

henryiii commented Sep 8, 2022

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

@zheddie
Copy link
Copy Markdown
Contributor Author

zheddie commented Sep 13, 2022

@mayeut , any comments ? thanks.

@zheddie
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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