-
-
Notifications
You must be signed in to change notification settings - Fork 23
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 telescope+libretls port #196
base: master
Are you sure you want to change the base?
Conversation
-int safe_mode; | ||
+int safe_mode = 0; |
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 shouldn't do anything since global vars are initialized to zero anyway.
@@ -794,7 +794,34 @@ packages: | |||
- args: ['make', 'install'] | |||
environ: | |||
DESTDIR: '@THIS_COLLECT_DIR@' | |||
|
|||
- name: libretls |
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 port lacks meta data. Is it necessary to use this package? We already have OpenSSL.
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.
libretls is a libtls reimplementation of libtls for openssl
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.
Aside from missing metadata and architecture labels, ports should have one empty line between them. And yes, we need libretls
or make telescope
link against libressl
, which I'm against. Gentoo has libretls
in their repo's too, so this is fine.
- while ((ch = getopt_long(argc, argv, opts, longopts, NULL)) != -1) { | ||
+ optind = 0; | ||
+ while ((ch = getopt(argc, argv, opts)) != -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.
Why is this done?
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.
Working around unimplemented/incorrectly implemented functionality in getopt_long
. The optind = 0
can be dropped, though.
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.
Then getopt_long should be fixed instead of hacking around such issues here.
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.
See managarm/mlibc#438 for the fix.
- if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pipe2net) == -1) | ||
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, pipe2net) == -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.
Why is this done? If we're missing PF_UNSPEC
, adding it to mlibc should be simple.
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.
We have it, however Managarm code asserts that the protocol (3rd argument) is 0. We should change the Managarm code to allow PF_UNSPEC (which is basically pick what you think is best afaik).
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.
upstream is wrong here afaik, from posix:
protocol Specifies a particular protocol to be used with the socket. Specifying a protocol of 0 causes socket() to use an unspecified default
protocol appropriate for the requested socket type.
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 a first attempt this is quite decent, I've added my comments that others hadn't made yet.
@@ -794,7 +794,34 @@ packages: | |||
- args: ['make', 'install'] | |||
environ: | |||
DESTDIR: '@THIS_COLLECT_DIR@' | |||
|
|||
- name: libretls |
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.
Aside from missing metadata and architecture labels, ports should have one empty line between them. And yes, we need libretls
or make telescope
link against libressl
, which I'm against. Gentoo has libretls
in their repo's too, so this is fine.
- args: ['make', '-j@PARALLELISM@'] | ||
- args: ['make', 'install'] | ||
environ: | ||
DESTDIR: '@THIS_COLLECT_DIR@' |
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.
Same here, newline after a port recipe.
configure: | ||
- args: | ||
- '@THIS_SOURCE_DIR@/configure' | ||
- '--host=x86_64-managarm' |
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.
We should use - '--host=@OPTION:arch-triple@'
instead of hardcoding architectures here.
@@ -53,3 +53,40 @@ packages: | |||
- args: ['make', 'DESTDIR=@THIS_COLLECT_DIR@', 'install'] | |||
environ: | |||
DESTDIR: '@THIS_COLLECT_DIR@' | |||
- name: telescope |
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.
Same here, a newline should be added between ports.
Gonna hold of on this until the various mlibc things are merged |
No description provided.