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 to upstream 1.13.0 #2

Closed
wants to merge 3 commits into from

Conversation

andreittr
Copy link
Contributor

Description of changes

Update Google Test to upstream 1.13.0, as well as clean up build scripts & documentation.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 28, 2023
@razvand razvand added the enhancement New feature or request label Jun 28, 2023
@razvand razvand self-assigned this Jun 28, 2023
Copy link

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

The changes look good, thanks @andreittr. Regarding the documentation part, however, the README specifies the order of the libraries which we don't use anymore (and has newlib mentioned as well - kinda obsolete).

AFAIK, the latest correct order is

LIBS := $(UK_LIBS)/lib-libcxxabi:$(UK_LIBS)/lib-libcxx:$(UK_LIBS)/lib-compiler-rt:$(UK_LIBS)/lib-libunwind:$(UK_LIBS)/lib-musl:$(UK_LIBS)/lib-googletest

Do you mind changing that too @andreittr? Actually the whole README should be updated to have the md format. I can add a commit to achieve that, if you find it too cumbersome.

@andreittr
Copy link
Contributor Author

The changes look good, thanks @andreittr. Regarding the documentation part, however, the README specifies the order of the libraries which we don't use anymore (and has newlib mentioned as well - kinda obsolete).

AFAIK, the latest correct order is

LIBS := $(UK_LIBS)/lib-libcxxabi:$(UK_LIBS)/lib-libcxx:$(UK_LIBS)/lib-compiler-rt:$(UK_LIBS)/lib-libunwind:$(UK_LIBS)/lib-musl:$(UK_LIBS)/lib-googletest

Do you mind changing that too @andreittr? Actually the whole README should be updated to have the md format. I can add a commit to achieve that, if you find it too cumbersome.

Good point @mariasfiraiala , thanks! I'll add a commit updating the README to be roughly in line with ones in more up-to-date libs. I'll also simplify the lib order paragraph to just "put at the end of the lib list".

Copy link

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

Thanks @andreittr, 2 last comments from me below. After this, I think we should merge this.

README.md Outdated
Please refer to the `README.md` as well as the documentation in the
`doc/` subdirectory of the main unikraft repository for further
information.
Being a C++ library, libcxx is required.

Choose a reason for hiding this comment

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

Suggested change
Being a C++ library, libcxx is required.
Being a C++ library, `libcxx` is required.

README.md Outdated
`doc/` subdirectory of the main unikraft repository for further
information.
Being a C++ library, libcxx is required.
This library should come at the end of the dependency list.

Choose a reason for hiding this comment

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

Suggested change
This library should come at the end of the dependency list.
`lib-googletest` should come at the end of the dependency list.

It isn't clear enough what library should come last.

Reformat to Markdowm.
CONTRIBUTING: remove obsolete references, point users at unikraft website
README: clean up & simplify wording, point users at website

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Copy link

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

All good now, thanks a lot for the effort, @andreittr!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

Copy link

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #2
unikraft-bot pushed a commit that referenced this pull request Aug 11, 2023
Reformat to Markdowm.
CONTRIBUTING: remove obsolete references, point users at unikraft website
README: clean up & simplify wording, point users at website

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
Reviewed-by: Maria Sfiraiala <maria.sfiraiala@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #2
@andreittr andreittr deleted the ttr/gtest-113 branch August 11, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/merged enhancement New feature or request
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

4 participants