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

Add Makefile and update Makefile.uk #9

Closed
wants to merge 2 commits into from

Conversation

eduardvintila
Copy link
Member

@eduardvintila eduardvintila commented Jul 26, 2023

This PR is part of a series of PRs that work in conjunction for the update of Go support to 1.18:

For testing, make sure to pass to QEMU the -cpu host argument (this is necessary because the paging API and ukvmem are required by libgo as part of Virtual Address space management, and somehow the default CPU just won't handle 1GB pages).

Only GCC12 and x86 is supported for the moment.

Copy link

@RaduNichita RaduNichita left a comment

Choose a reason for hiding this comment

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

It works on my side, many thanks @eduardvintila :godmode:

I left two questions more out of curiosity, I will approve after receiving your input of those.

Makefile Outdated
UK_ROOT ?= $(PWD)/.unikraft/unikraft
UK_LIBS ?= $(PWD)/.unikraft/libs

LIBS-y :=

Choose a reason for hiding this comment

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

Why did you put the libraries like so and not on the same line?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reason, it was easier to remove and add libraries that way 😆

@@ -1,3 +1,3 @@
$(eval $(call addlib,apphelloworldgo))
$(eval $(call addgoapp,apphelloworldgo))

Choose a reason for hiding this comment

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

Why do you have this change here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because libgo has custom build mechanics, given by this PR from Unikraft core and this rule from libgo

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

@eduardvintila looks good, few comments:

  • Use LIBS instead of LIBS-y, in order to be consistent with other applications.
  • State why changing the addlib rule to addgoapp was necessary in the commit description.

Golang applications which use the `libgo` external library have
custom build mechanics, where the `addlib` rule is replaced by
either `addgolib` (for GO packages used as libraries), or `addgoapp`.
In this case, we need to call the `addgoapp` Makefile rule to
indicate that we're building a GO app which provides the main function.

Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>
Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>
@eduardvintila
Copy link
Member Author

@StefanJum I've added an explanation and replaced LIBS-y with LIBS. @RaduNichita, I've left the libraries only on one line :)

Copy link

@RaduNichita RaduNichita 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 with me now.

Reviewed-by: Radu Nichita radunichita99@gmail.com

Copy link
Member

@StefanJum StefanJum 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, thanks.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@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 7, 2023
Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>
Reviewed-by: Radu Nichita <radunichita99@gmail.com>
Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com>
Approved-by: Razvan Deaconescu <razvand@unikraft.io>
Tested-by: Unikraft CI <monkey@unikraft.io>
GitHub-Closes: #9
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.

5 participants