-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
New Package: odin-0.10.0 #12322
New Package: odin-0.10.0 #12322
Conversation
srcpkgs/odin/template
Outdated
pkgname=odin | ||
version=0.10.0 | ||
revision=1 | ||
archs="x86_64-musl x86_64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be abbreviated with archs="x86_64*"
.
srcpkgs/odin/template
Outdated
build_style="gnu-makefile" | ||
hostmakedepends="pkg-config clang llvm" | ||
depends="clang" | ||
short_desc="Fast. concise, readable, pragmatic and open programming language" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: .
instead of ,
srcpkgs/odin/template
Outdated
depends="clang" | ||
short_desc="Fast. concise, readable, pragmatic and open programming language" | ||
maintainer="Logen Kain <logen@sudotask.com>" | ||
license="BSD" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the SPDX license identifier.
srcpkgs/odin/template
Outdated
homepage="https://odin-lang.org" | ||
distfiles="https://github.com/odin-lang/Odin/archive/v${version}.tar.gz" | ||
checksum=04a1c9e8719281a598cac5e9cc3e5ca2316cc038a8b8f9ae43f514aba967c635 | ||
nopie=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at other packages, the convention here seems to be nopie=yes
.
srcpkgs/odin/template
Outdated
distfiles="https://github.com/odin-lang/Odin/archive/v${version}.tar.gz" | ||
checksum=04a1c9e8719281a598cac5e9cc3e5ca2316cc038a8b8f9ae43f514aba967c635 | ||
nopie=1 | ||
make_build_args+=" debug" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For specifying the build target, you can use make_build_target="debug"
.
srcpkgs/odin/template
Outdated
nopie=1 | ||
make_build_args+=" debug" | ||
|
||
do_build() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let the gnu-makefile
buildstyle handle do_build()
: There is the make_use_env
variable for doing exactly this do_build()
.
srcpkgs/odin/template
Outdated
vlicense LICENSE | ||
ln -s /opt/odin/odin ${DESTDIR}/usr/bin/odin | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty line can be removed.
There are a lot of unneeded files ( |
Ah, I didn't get that last message till I pushed that last commit. I'll get right on it. |
Alright, fixed. Thanks a lot, it's been a while since I've put together a package. You're doing great work. |
srcpkgs/odin/template
Outdated
@@ -2,25 +2,20 @@ | |||
pkgname=odin | |||
version=0.10.0 | |||
revision=1 | |||
archs="x86_64-musl x86_64" | |||
archs="x86_64" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note the *
from my first review comment, so archs="x86_64*"
.
@@ -20,7 +20,9 @@ make_use_env=yes | |||
do_install() { | |||
vmkdir opt/odin | |||
vmkdir usr/bin | |||
vcopy "*" opt/odin | |||
vcopy "shared" opt/odin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? It just includes an empty .gitkeep
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only just started messing around with the language, but their github readme explicitly states that core and shared library folders need to stay relative to the executable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like to be a placeholder where .so
(or .odin
?) files can be dropped for importing from within odin, so maybe just keep this.
(I had a concern that odin maybe uses this directory for temporary storing shared objects before linking the final binary. This would be problematic with a root-owned /opt
.)
While trying to compile the |
I thought I tested that, but llvm8 was still installed, ah well. |
Looks good so far. To finalize, please squash all commits into one named like the initial commit: |
odin: revise template to standards odin: limit installed files to what is needed odin: fix xlint errors odin: fix target archs odin: add llvm depend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your contribution!
I also agree and try to remain lower-case when submitting packages. Things like libGL don't bother me though. |
Closing to clean up pull requests since it's clearly not going to be merged (probably need to update it by now anyway) |
Odin doesn't seem to have an installer yet and it requires the executable to be local to it's shared and core folders.
I could change it up so it only copies the binary and the two required folders, but odin is still early days so I figure I should leave it all there in case anyone wants to look at it.
EDIT:
Or if future updates requires something else to be kept local to the executable.