-
Notifications
You must be signed in to change notification settings - Fork 68
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
Improve handling of systemd activation #50
Conversation
Signed-off-by: Laurențiu Nicola <[email protected]>
499aa77
to
af2888b
Compare
r? @hug-dev |
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.
Thank you very much @lnicola ! The new function of sd_notify
, listen_fds
is pretty good and allows us to remove the compile-time flag at many places.
I made some comment about licensing mainly but it all looks good to me.
@ionut-arm The systemd-daemon
flag is now only used in the beginning of the main
to remove timestamps of logs. Do you think we could remove that flag completely by adding logging options to the configuration file instead?
Also, we should make a note to try to compile our code on Windows and add the target OS conditionial compilation flags where it is needed. Currently the only listener we support is a Unix Domain socket listener so that is not an emergency.
@@ -128,19 +128,19 @@ You will need to understand the [**wire protocol specification**](docs/wire_prot | |||
The software is provided under Apache-2.0. Contributions to this project are accepted under the same license. | |||
|
|||
This project uses the following third party crates: | |||
* serde (Apache-2.0) | |||
* serde (MIT and Apache-2.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.
When the crate choose a double-licensing option (Apache-2.0 OR MIT), as the following (example uuid
):
License
Licensed under either of:
- Apache License, Version 2.0, (LICENSE-APACHE or https://www.apache.org/licenses/LICENSE-2.0)
- MIT license (LICENSE-MIT or https://opensource.org/licenses/MIT)
at your option.Contribution
Unless you explicitly state otherwise, any contribution intentionally submitted for inclusion in the work by you, as defined in the Apache-2.0 license, shall be dual licensed as above, without any additional terms or conditions.
We explicitely make a choice and choose in that file to use Apache-2.0 license. If it is AND, we keep the two licenses.
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 can revert the licensing change, but that sounds like a false dichotomy. All of those crates are dual-licensed, in the sense that you can pick either of them. The difference you noticed is in the manifest. People used to write MIT/Apache-2.0
, but now MIT OR Apache-2.0
is preferred because it's "more machine-readable". Both are equivalent, see the comment in https://doc.rust-lang.org/stable/cargo/reference/manifest.html#package-metadata.
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.
Thanks for the pointer. What I understand from that is that is is more explicit to use AND or OR instead of a slash (/
) but not that they both mean the same thing 😛
But I do not know if the licenses are for use of the library or contributions to it, and if it is for contributions that means that there could be files both licensed under MIT and Apache-2.0 in the same library.
In that case, it is probably better to use AND everywhere as you changed.
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 think OR
would be a better fit.
But I do not know if the licenses are for use of the library or contributions to it, and if it is for contributions that means that there could be files both licensed under MIT and Apache-2.0 in the same library.
It's for the crate, and dual-licensing means that the user of the crate gets to pick one of the licenses (or both) that fits their project better. By contributing to a crate you implicitly admit that your changes will be licensed under the crate's terms (e.g. both MIT and Apache-2.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.
Would not it feel weird to have OR on this file as we are building a binary and not re-distributing the crates? As in, we are the last one in the chain so should not we make that decision between Apache-2.0 or MIT?
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 think it makes sense to say "PARSEC is licensed under Apache-2.0 and uses the following dependencies, with their associated licenses", but I am not a lawyer :-).
@@ -31,7 +31,7 @@ static SOCKET_PATH: &str = "/tmp/security-daemon-socket"; | |||
/// | |||
/// Only works on Unix systems. | |||
pub struct DomainSocketListener { | |||
listener: Option<UnixListener>, |
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.
Makes sense to remove that to me! As the DomainSocketListener
can only be instanciated by the builder, this will never be None
.
Also
This is fine as the CI checks passed but is it because you need to install |
Signed-off-by: Laurențiu Nicola <[email protected]>
af2888b
to
d07bf5e
Compare
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.
Thanks for offering the changes!
I think it was a dirty
and
I saw that CI was working, but I don't know how. The |
Hmm #[repr(C)]
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug)]
pub struct mbedtls_md2_context {
pub cksum: [::std::os::raw::c_uchar; 16usize],
pub state: [::std::os::raw::c_uchar; 48usize],
pub buffer: [::std::os::raw::c_uchar; 16usize],
pub left: usize,
} and now I have the same error:
Does your |
Also do you have the same copy of |
Yes, my #[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct mbedtls_md2_context {
pub cksum: [::std::os::raw::c_uchar; 16usize],
>> pub state: [::std::os::raw::c_uchar; 48usize],
pub buffer: [::std::os::raw::c_uchar; 16usize],
pub left: usize,
} |
Ah! I switched no |
Yeah, sorry, I am on nightly, but beta seems to give the same result. I'm not sure how it affects the |
sd-notify
dependency is now mandatory, but should be harmless on non-systemd
distrossd-notify
dependencythe socket is marked as non-blocking (it wasn't before)Option
The
systemd-daemon
feature is still available to configure whether timestamps are included in the logs. That could probably be handled better by checking whetherstderr
is a TTY.Untested, the build script fails to link on my system even when disabling the
mbed
feature.