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

update OSInfo.java #691

Merged
merged 1 commit into from
Jul 28, 2022
Merged

update OSInfo.java #691

merged 1 commit into from
Jul 28, 2022

Conversation

ChanthMiao
Copy link
Contributor

  • add ppc64el/ppc64le mapping

  • recompile OSInfo.class with JDK 1.8

  • delete old 'lib/org/sqlite/OSInfo.class'

@gotson
Copy link
Collaborator

gotson commented Nov 21, 2021

The class files should not even be here, I think they are not used and should be deleted completely.

@ChanthMiao
Copy link
Contributor Author

The class files should not even be here, I think they are not used and should be deleted completely.

ifndef OS_NAME
OS_NAME := $(shell $(JAVA) -cp lib $(OSINFO_CLASS) --os)
endif
ifndef OS_ARCH
OS_ARCH := $(shell $(JAVA) -cp lib $(OSINFO_CLASS) --arch)

It seems that the class files are referenced here.

@gotson
Copy link
Collaborator

gotson commented Dec 2, 2021

Looks like it's used only when OS_NAME and OS_ARCH are not passed, so probably when using make native.

IMHO we should compile osinfo if needed when those variables are not needed, instead of committing them to source control.

@ChanthMiao
Copy link
Contributor Author

You are right. We should not commit class files to git repo

@ChanthMiao
Copy link
Contributor Author

@gotson I just made a new PR #697, which deleted precompiled OSInfo.class safely.

@gotson
Copy link
Collaborator

gotson commented Dec 5, 2021

@ChanthMiao looking at the existing code in master, it should work fine.

The only problem is that that class files must have been committed by error.

I would suggest a single PR that deletes the existing class, and adds a .gitignore entry to avoid committing again in the future.

Separating your PR about ppc64le in another PR would also make the PR clearer and easier to merge IMHO.

@ChanthMiao
Copy link
Contributor Author

ChanthMiao commented Dec 5, 2021

@gotson It was not commited by error. The OSInfo.class was commit by f09fb1e

Then it Integrated by the other commit 6d3bfb7

@ChanthMiao
Copy link
Contributor Author

ChanthMiao commented Dec 5, 2021

@ChanthMiao looking at the existing code in master, it should work fine.

The only problem is that that class files must have been committed by error.

I would suggest a single PR that deletes the existing class, and adds a .gitignore entry to avoid committing again in the future.

Separating your PR about ppc64le in another PR would also make the PR clearer and easier to merge IMHO.

The new PR, which deletes the existing class is here #698.

If it merged, I would make the other one to add ppc64le mapping

@gotson
Copy link
Collaborator

gotson commented Jul 28, 2022

@ChanthMiao i will merge #715 soon that will handle OSInfo on the fly compilation.

Can you cleanup this PR to keep only the ppc64el/ppc64le mapping ? This should help to close #450

@ChanthMiao
Copy link
Contributor Author

Can you cleanup this PR to keep only the ppc64el/ppc64le mapping ? This should help to close #450

I may do it this weekend.

* add ppc64el/ppc64le mapping

Signed-off-by: Changwei Miao <chanthmiao@outlook.com>
@ChanthMiao
Copy link
Contributor Author

@gotson Done!

@gotson gotson merged commit 6ee09e1 into xerial:master Jul 28, 2022
@ChanthMiao ChanthMiao deleted the fix-ppc64-load branch July 28, 2022 14:00
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.

2 participants