Require enclave upon rmw_init() call.

[hidmic]

Follow-up after #243. Enclave is within rmw_init_options_t but it isn't an option to provide one.

[hidmic]

Considering that it is mandatory, I'd rather make enclave an argument of rmw_init(). However, this work, i.e. that of improving rmw test coverage, must be available for Foxy and so I can't change the signature. CC @chapulina.

[clalancette]

Considering that it is mandatory, I'd rather make enclave an argument of rmw_init(). However, this work, i.e. that of improving rmw test coverage, must be available for Foxy and so I can't change the signature. CC @chapulina.

For what it's worth, I think we should do the right thing for dealing with Galactic, and then worry about backporting it (which may need a different solution). If that means breaking the API here, then I'd be for it (though we also have to worry about deprecating the old API).

[hidmic]

@clalancette that's fair. But because we have to backport it, I'll get documentation and tests right for Foxy first. Then we can change the API and propagate all changes. Do you agree?

[ivanpauno]

Considering that it is mandatory, I'd rather make enclave an argument of rmw_init(). However, this work, i.e. that of improving rmw test coverage, must be available for Foxy and so I can't change the signature. CC @chapulina.

I don't see a benefit of adding an extra argument.
I prefer passing only the init_options, so we don't have to extend the signature each time we add one.

For what it's worth, I think we should do the right thing for dealing with Galactic, and then worry about backporting it (which may need a different solution). If that means breaking the API here, then I'd be for it (though we also have to worry about deprecating the old API).

👍

[hidmic]

I don't see a benefit of adding an extra argument. I prefer passing only the init_options, so we don't have to extend the signature each time we add one.

I understand how convenient it is, but IMHO it is conceptually inaccurate. As it stands, enclave is not an option, just like a namespace isn't an option when creating nodes. I'd rather leverage the compiler to check that all mandatory arguments have been provided instead of relying on runtime checks.

[ivanpauno]

I understand how convenient it is, but IMHO it is conceptually inaccurate. As it stands, enclave is not an option, just like a namespace isn't an option when creating nodes. I'd rather leverage the compiler to check that all mandatory arguments have been provided instead of relying on runtime checks.

I don't get the point. If the API would be:

rmw_init(rmw_context_t * context, rmw_init_options_t *init_options, const char * enclave_name);

enclave_name can still be NULL, the compiler can't check that for you.

[clalancette]

enclave_name can still be NULL, the compiler can't check that for you.

While that is true, that is a quirk of this code being in C. If this was in C++, for instance, that argument would likely be a const std::string &, so you would always have to pass something there. In either case, you have to check for string validity at runtime anyway.

[hidmic]

While that is true, that is a quirk of this code being in C.

This. Even though a check for nullity is still required because of language support, the API would better convey the fact that an enclave_name is expected.

[hidmic]

Ok, going in with this.

[ivanpauno]

This. Even though a check for nullity is still required because of language support, the API would better convey the fact that an enclave_name is expected.

Ok, I think it's a valid change, though I don't see a huge win in C.
It's true that currently the "zero initialized" init options are invalid, and that might be a bit confusing.

[jacobperron]

Looks like changes to from the related PRs caused rmw_implementation tests to start failing: https://ci.ros2.org/view/nightly/job/nightly_linux_repeated/1927/testReport/

[jacobperron]

Looks like changes to from the related PRs caused rmw_implementation tests to start failing: https://ci.ros2.org/view/nightly/job/nightly_linux_repeated/1927/testReport/

Looks like ros2/rmw_implementation#113 should be merged to resolve the failures.