[2/3] scanner: Allow adding a prefix to exported symbols

Submitted by Jonas Ådahl on July 3, 2017, 9:16 a.m.

Details

Message ID 20170703091646.24246-2-jadahl@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Jonas Ådahl July 3, 2017, 9:16 a.m.
Two different protocols may use interfaces with identical names.
Implementing support for both those protocols would result in symbol
clashes, as wayland-scanner generates symbols from the interface names.

Make it possible to avoiding these clashes by adding a way to add a
prefix to the symbols generated by wayland-scanner. Implementations
(servers and clients) can then use these prefix:ed symbols to implement
different objects with the same name.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
---

Something like this would be needed if a compositor/client wants to implement
xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
xdg-shell stable does not have this issue.

See issue raised here:
https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html


Jonas


 src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 73 insertions(+), 21 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/scanner.c b/src/scanner.c
index 517068c..00b5a84 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -228,6 +228,7 @@  struct entry {
 struct parse_context {
 	struct location loc;
 	XML_Parser parser;
+	char *symbol_prefix;
 	struct protocol *protocol;
 	struct interface *interface;
 	struct message *message;
@@ -1041,7 +1042,9 @@  emit_type(struct arg *a)
 }
 
 static void
-emit_stubs(struct wl_list *message_list, struct interface *interface)
+emit_stubs(struct parse_context *ctx,
+	   struct wl_list *message_list,
+	   struct interface *interface)
 {
 	struct message *m;
 	struct arg *a, *ret;
@@ -1168,11 +1171,12 @@  emit_stubs(struct wl_list *message_list, struct interface *interface)
 			printf("\tstruct wl_proxy *%s;\n\n"
 			       "\t%s = wl_proxy_marshal_constructor("
 			       "(struct wl_proxy *) %s,\n"
-			       "\t\t\t %s_%s, &%s_interface",
+			       "\t\t\t %s_%s, &%s%s_interface",
 			       ret->name, ret->name,
 			       interface->name,
 			       interface->uppercase_name,
 			       m->uppercase_name,
+			       ctx->symbol_prefix,
 			       ret->interface_name);
 		} else {
 			/* No args have type="new_id" */
@@ -1507,7 +1511,7 @@  emit_mainpage_blurb(const struct protocol *protocol, enum side side)
 }
 
 static void
-emit_header(struct protocol *protocol, enum side side)
+emit_header(struct parse_context *ctx, struct protocol *protocol, enum side side)
 {
 	struct interface *i, *i_next;
 	struct wl_array types;
@@ -1576,7 +1580,7 @@  emit_header(struct protocol *protocol, enum side side)
 			format_text_to_comment(i->description->text, false);
 		printf(" */\n");
 		printf("extern const struct wl_interface "
-		       "%s_interface;\n", i->name);
+		       "%s%s_interface;\n", ctx->symbol_prefix, i->name);
 	}
 
 	printf("\n");
@@ -1596,7 +1600,7 @@  emit_header(struct protocol *protocol, enum side side)
 			emit_opcodes(&i->request_list, i);
 			emit_opcode_versions(&i->event_list, i);
 			emit_opcode_versions(&i->request_list, i);
-			emit_stubs(&i->request_list, i);
+			emit_stubs(ctx, &i->request_list, i);
 		}
 
 		free_interface(i);
@@ -1619,7 +1623,9 @@  emit_null_run(struct protocol *protocol)
 }
 
 static void
-emit_types(struct protocol *protocol, struct wl_list *message_list)
+emit_types(struct parse_context *ctx,
+	   struct protocol *protocol,
+	   struct wl_list *message_list)
 {
 	struct message *m;
 	struct arg *a;
@@ -1639,7 +1645,8 @@  emit_types(struct protocol *protocol, struct wl_list *message_list)
 			case NEW_ID:
 			case OBJECT:
 				if (a->interface_name)
-					printf("\t&%s_interface,\n",
+					printf("\t&%s%s_interface,\n",
+					       ctx->symbol_prefix,
 					       a->interface_name);
 				else
 					printf("\tNULL,\n");
@@ -1713,7 +1720,7 @@  emit_messages(struct wl_list *message_list,
 }
 
 static void
-emit_code(struct protocol *protocol)
+emit_code(struct parse_context *ctx, struct protocol *protocol)
 {
 	struct interface *i, *next;
 	struct wl_array types;
@@ -1738,7 +1745,8 @@  emit_code(struct protocol *protocol)
 	wl_array_for_each(p, &types) {
 		if (prev && strcmp(*p, prev) == 0)
 			continue;
-		printf("extern const struct wl_interface %s_interface;\n", *p);
+		printf("extern const struct wl_interface %s%s_interface;\n",
+		       ctx->symbol_prefix, *p);
 		prev = *p;
 	}
 	wl_array_release(&types);
@@ -1747,8 +1755,8 @@  emit_code(struct protocol *protocol)
 	printf("static const struct wl_interface *types[] = {\n");
 	emit_null_run(protocol);
 	wl_list_for_each(i, &protocol->interface_list, link) {
-		emit_types(protocol, &i->request_list);
-		emit_types(protocol, &i->event_list);
+		emit_types(ctx, protocol, &i->request_list);
+		emit_types(ctx, protocol, &i->event_list);
 	}
 	printf("};\n\n");
 
@@ -1758,9 +1766,9 @@  emit_code(struct protocol *protocol)
 		emit_messages(&i->event_list, i, "events");
 
 		printf("WL_EXPORT const struct wl_interface "
-		       "%s_interface = {\n"
+		       "%s%s_interface = {\n"
 		       "\t\"%s\", %d,\n",
-		       i->name, i->name, i->version);
+		       ctx->symbol_prefix, i->name, i->name, i->version);
 
 		if (!wl_list_empty(&i->request_list))
 			printf("\t%d, %s_requests,\n",
@@ -1790,6 +1798,35 @@  free_protocol(struct protocol *protocol)
 	free_description(protocol->description);
 }
 
+static bool
+is_valid_symbol_prefix(char *symbol_prefix)
+{
+	int i, len;
+
+	len = strlen(symbol_prefix);
+	for (i = 0; i < len; i++) {
+		char c = symbol_prefix[i];
+
+		if (i == 0) {
+			if (isalpha(c))
+				continue;
+
+			return false;
+		} else {
+			if (isalpha(c))
+				continue;
+			if (isdigit(c))
+				continue;
+			if (c == '_')
+				continue;
+
+			return false;
+		}
+	}
+
+	return true;
+}
+
 int main(int argc, char *argv[])
 {
 	struct parse_context ctx;
@@ -1802,6 +1839,7 @@  int main(int argc, char *argv[])
 	bool core_headers = false;
 	bool version = false;
 	bool fail = false;
+	char *symbol_prefix = NULL;
 	int opt;
 	enum {
 		CLIENT_HEADER,
@@ -1810,14 +1848,15 @@  int main(int argc, char *argv[])
 	} mode;
 
 	static const struct option options[] = {
-		{ "help",              no_argument, NULL, 'h' },
-		{ "version",           no_argument, NULL, 'v' },
-		{ "include-core-only", no_argument, NULL, 'c' },
-		{ 0,                   0,           NULL, 0 }
+		{ "help",              no_argument,       NULL, 'h' },
+		{ "version",           no_argument,       NULL, 'v' },
+		{ "include-core-only", no_argument,       NULL, 'c' },
+		{ "symbol-prefix",     required_argument, NULL, 'p' },
+		{ 0,                   0,                 NULL, 0 }
 	};
 
 	while (1) {
-		opt = getopt_long(argc, argv, "hvc", options, NULL);
+		opt = getopt_long(argc, argv, "hvcp:", options, NULL);
 
 		if (opt == -1)
 			break;
@@ -1832,6 +1871,9 @@  int main(int argc, char *argv[])
 		case 'c':
 			core_headers = true;
 			break;
+		case 'p':
+			symbol_prefix = strdup(optarg);
+			break;
 		default:
 			fail = true;
 			break;
@@ -1858,6 +1900,14 @@  int main(int argc, char *argv[])
 	else
 		usage(EXIT_FAILURE);
 
+	if (!symbol_prefix)
+		symbol_prefix = strdup("");
+	if (!is_valid_symbol_prefix(symbol_prefix)) {
+		fprintf(stderr, "Symbol prefix '%s' not valid\n",
+			symbol_prefix);
+		exit(EXIT_FAILURE);
+	}
+
 	if (argc == 3) {
 		input_filename = argv[1];
 		input = fopen(input_filename, "r");
@@ -1882,6 +1932,7 @@  int main(int argc, char *argv[])
 	/* initialize context */
 	memset(&ctx, 0, sizeof ctx);
 	ctx.protocol = &protocol;
+	ctx.symbol_prefix = symbol_prefix;
 	if (input == stdin)
 		ctx.loc.filename = "<stdin>";
 	else
@@ -1931,16 +1982,17 @@  int main(int argc, char *argv[])
 
 	switch (mode) {
 		case CLIENT_HEADER:
-			emit_header(&protocol, CLIENT);
+			emit_header(&ctx, &protocol, CLIENT);
 			break;
 		case SERVER_HEADER:
-			emit_header(&protocol, SERVER);
+			emit_header(&ctx, &protocol, SERVER);
 			break;
 		case CODE:
-			emit_code(&protocol);
+			emit_code(&ctx, &protocol);
 			break;
 	}
 
+	free(symbol_prefix);
 	free_protocol(&protocol);
 	fclose(input);
 

Comments

On Mon,  3 Jul 2017 17:16:45 +0800
Jonas Ådahl <jadahl@gmail.com> wrote:

> Two different protocols may use interfaces with identical names.
> Implementing support for both those protocols would result in symbol
> clashes, as wayland-scanner generates symbols from the interface names.
> 
> Make it possible to avoiding these clashes by adding a way to add a
> prefix to the symbols generated by wayland-scanner. Implementations
> (servers and clients) can then use these prefix:ed symbols to implement
> different objects with the same name.
> 
> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> ---
> 
> Something like this would be needed if a compositor/client wants to implement
> xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> xdg-shell stable does not have this issue.
> 
> See issue raised here:
> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> 
> 
> Jonas
> 
> 
>  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 73 insertions(+), 21 deletions(-)


Hi,

while this seems to change the ABI symbol names, it does not change the
names in the documentation, and it does not change the names of
#defines of enums, or the inline functions. That means that this is not
enough to fulfill the purpose: being able to use two similarly named
but different protocols by adding a prefix.

For the idea:
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

But I think it needs to apply the prefix to *everything*, both ABI and
API.


Thanks,
pq
On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
> On Mon,  3 Jul 2017 17:16:45 +0800
> Jonas Ådahl <jadahl@gmail.com> wrote:
> 
> > Two different protocols may use interfaces with identical names.
> > Implementing support for both those protocols would result in symbol
> > clashes, as wayland-scanner generates symbols from the interface names.
> > 
> > Make it possible to avoiding these clashes by adding a way to add a
> > prefix to the symbols generated by wayland-scanner. Implementations
> > (servers and clients) can then use these prefix:ed symbols to implement
> > different objects with the same name.
> > 
> > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> > ---
> > 
> > Something like this would be needed if a compositor/client wants to implement
> > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> > xdg-shell stable does not have this issue.
> > 
> > See issue raised here:
> > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> > 
> > 
> > Jonas
> > 
> > 
> >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 73 insertions(+), 21 deletions(-)
> 
> 
> Hi,
> 
> while this seems to change the ABI symbol names, it does not change the
> names in the documentation, and it does not change the names of
> #defines of enums, or the inline functions. That means that this is not
> enough to fulfill the purpose: being able to use two similarly named
> but different protocols by adding a prefix.

The idea I had was rather that one would avoid changing any names on
non-symbols. It'd still be possible to implement both by doing it in
separate C files. I can see the point in adding the prefix on everything
though, so I'll provide a patch for that.


Jonas

> 
> For the idea:
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> 
> But I think it needs to apply the prefix to *everything*, both ABI and
> API.
> 
> 
> Thanks,
> pq
On Tue, 25 Jul 2017 15:25:58 +0800
Jonas Ådahl <jadahl@gmail.com> wrote:

> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
> > On Mon,  3 Jul 2017 17:16:45 +0800
> > Jonas Ådahl <jadahl@gmail.com> wrote:
> >   
> > > Two different protocols may use interfaces with identical names.
> > > Implementing support for both those protocols would result in symbol
> > > clashes, as wayland-scanner generates symbols from the interface names.
> > > 
> > > Make it possible to avoiding these clashes by adding a way to add a
> > > prefix to the symbols generated by wayland-scanner. Implementations
> > > (servers and clients) can then use these prefix:ed symbols to implement
> > > different objects with the same name.
> > > 
> > > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> > > ---
> > > 
> > > Something like this would be needed if a compositor/client wants to implement
> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> > > xdg-shell stable does not have this issue.
> > > 
> > > See issue raised here:
> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> > > 
> > > 
> > > Jonas
> > > 
> > > 
> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> > >  1 file changed, 73 insertions(+), 21 deletions(-)  
> > 
> > 
> > Hi,
> > 
> > while this seems to change the ABI symbol names, it does not change the
> > names in the documentation, and it does not change the names of
> > #defines of enums, or the inline functions. That means that this is not
> > enough to fulfill the purpose: being able to use two similarly named
> > but different protocols by adding a prefix.  
> 
> The idea I had was rather that one would avoid changing any names on
> non-symbols. It'd still be possible to implement both by doing it in
> separate C files. I can see the point in adding the prefix on everything
> though, so I'll provide a patch for that.
> 

Hi,

recapping the discussion from IRC, we pretty much agreed that prefixing
is not a nice solution. Jonas found out that we cannot actually prefix
everything, because there usually are references to other protocol
things (like you would never want to prefix wl_surface). So it becomes
very hard to prefix things appropriately.

The alternative we discussed is solving a different problem: scanner
makes all the public symbols in the generated .c files WL_EXPORT, which
makes them leak into DSO ABI, which is bad. In my opinion, it should
have never happened in the first place. But we missed it, and now it has
spread, so we cannot just fix scanner to stop exporting, the decision
must be with the consumers. So we need a scanner option to stop
exporting.

Quentin proposed we add a scanner option
--visibility={static|compiler|export}. It would affect all the symbols
exported from the generated .c files as follows:

- static: the symbols will be static.
- compiler: the symbols will get whatever the default visibility is
  with the compiler, i.e. not explicitly static and not exported
- export: the symbols are exported (this the old behaviour, and will be
  the default)

Obviously, the only way to actually make use of the 'static' option is
for the consumer to #include the generated .c file. It's ugly, yes, but
it solves the conflicting symbol names issue Jonas was looking into.

In my opinion, the prefixing approach where we still cannot prefix
everything in a way that one could use conflicting protocols in the
same compilation unit, and where e.g. the static inline functions are
not prefixed, is more ugly than the 'static' option.

We are going to need an option to stop the exports anyway, and it seems
like we can piggyback on that solution for the problem underlying the
prefixing proposal as well.


Thanks,
pq
Hi,

On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> recapping the discussion from IRC, we pretty much agreed that prefixing
> is not a nice solution. Jonas found out that we cannot actually prefix
> everything, because there usually are references to other protocol
> things (like you would never want to prefix wl_surface). So it becomes
> very hard to prefix things appropriately.
>
> The alternative we discussed is solving a different problem: scanner
> makes all the public symbols in the generated .c files WL_EXPORT, which
> makes them leak into DSO ABI, which is bad. In my opinion, it should
> have never happened in the first place. But we missed it, and now it has
> spread, so we cannot just fix scanner to stop exporting, the decision
> must be with the consumers. So we need a scanner option to stop
> exporting.
>
> Quentin proposed we add a scanner option
> --visibility={static|compiler|export}. It would affect all the symbols
> exported from the generated .c files as follows:
>
> - static: the symbols will be static.
> - compiler: the symbols will get whatever the default visibility is
>   with the compiler, i.e. not explicitly static and not exported
> - export: the symbols are exported (this the old behaviour, and will be
>   the default)
>
> Obviously, the only way to actually make use of the 'static' option is
> for the consumer to #include the generated .c file. It's ugly, yes, but
> it solves the conflicting symbol names issue Jonas was looking into.
>
> In my opinion, the prefixing approach where we still cannot prefix
> everything in a way that one could use conflicting protocols in the
> same compilation unit, and where e.g. the static inline functions are
> not prefixed, is more ugly than the 'static' option.
>
> We are going to need an option to stop the exports anyway, and it seems
> like we can piggyback on that solution for the problem underlying the
> prefixing proposal as well.

This sounds really good to me.

Unfortunately, the release just went out last night without waiting
for any of these patches (or even pinging to see what their status
was?), so I guess we're not able to make xdg-shell stable for another
cycle. >:(

It's either that or just merge it post-beta anyway - which I wouldn't
actually mind to be honest.

Cheers,
Daniel
On Wed, Jul 26, 2017 at 09:28:45AM +0100, Daniel Stone wrote:
> Hi,
> 
> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > recapping the discussion from IRC, we pretty much agreed that prefixing
> > is not a nice solution. Jonas found out that we cannot actually prefix
> > everything, because there usually are references to other protocol
> > things (like you would never want to prefix wl_surface). So it becomes
> > very hard to prefix things appropriately.
> >
> > The alternative we discussed is solving a different problem: scanner
> > makes all the public symbols in the generated .c files WL_EXPORT, which
> > makes them leak into DSO ABI, which is bad. In my opinion, it should
> > have never happened in the first place. But we missed it, and now it has
> > spread, so we cannot just fix scanner to stop exporting, the decision
> > must be with the consumers. So we need a scanner option to stop
> > exporting.
> >
> > Quentin proposed we add a scanner option
> > --visibility={static|compiler|export}. It would affect all the symbols
> > exported from the generated .c files as follows:
> >
> > - static: the symbols will be static.
> > - compiler: the symbols will get whatever the default visibility is
> >   with the compiler, i.e. not explicitly static and not exported
> > - export: the symbols are exported (this the old behaviour, and will be
> >   the default)
> >
> > Obviously, the only way to actually make use of the 'static' option is
> > for the consumer to #include the generated .c file. It's ugly, yes, but
> > it solves the conflicting symbol names issue Jonas was looking into.
> >
> > In my opinion, the prefixing approach where we still cannot prefix
> > everything in a way that one could use conflicting protocols in the
> > same compilation unit, and where e.g. the static inline functions are
> > not prefixed, is more ugly than the 'static' option.
> >
> > We are going to need an option to stop the exports anyway, and it seems
> > like we can piggyback on that solution for the problem underlying the
> > prefixing proposal as well.
> 
> This sounds really good to me.
> 
> Unfortunately, the release just went out last night without waiting
> for any of these patches (or even pinging to see what their status
> was?), so I guess we're not able to make xdg-shell stable for another
> cycle. >:(

Well, we can still, just that anyone wanting to implement it in
parallel to xdg-shell unstable v5 would have to wait.

> 
> It's either that or just merge it post-beta anyway - which I wouldn't
> actually mind to be honest.

That's an option too. I don't have any objections really.


Jonas

> 
> Cheers,
> Daniel
On Wed, 26 Jul 2017 18:39:52 +0800
Jonas Ådahl <jadahl@gmail.com> wrote:

> On Wed, Jul 26, 2017 at 09:28:45AM +0100, Daniel Stone wrote:
> > Hi,
> > 
> > On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:  

> > > Quentin proposed we add a scanner option
> > > --visibility={static|compiler|export}. It would affect all the symbols
> > > exported from the generated .c files as follows:
> > >
> > > - static: the symbols will be static.
> > > - compiler: the symbols will get whatever the default visibility is
> > >   with the compiler, i.e. not explicitly static and not exported
> > > - export: the symbols are exported (this the old behaviour, and will be
> > >   the default)

> > > We are going to need an option to stop the exports anyway, and it seems
> > > like we can piggyback on that solution for the problem underlying the
> > > prefixing proposal as well.  
> > 
> > This sounds really good to me.
> > 
> > Unfortunately, the release just went out last night without waiting
> > for any of these patches (or even pinging to see what their status
> > was?), so I guess we're not able to make xdg-shell stable for another
> > cycle. >:(  
> 
> Well, we can still, just that anyone wanting to implement it in
> parallel to xdg-shell unstable v5 would have to wait.
> 
> > 
> > It's either that or just merge it post-beta anyway - which I wouldn't
> > actually mind to be honest.  
> 
> That's an option too. I don't have any objections really.

I could be ok merging it before the first RC.

So are we sure enough that RC1 will give enough testing for the feature
to be carved in stone?


Thanks,
pq
On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Tue, 25 Jul 2017 15:25:58 +0800
> Jonas Ådahl <jadahl@gmail.com> wrote:
>
>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
>> > On Mon,  3 Jul 2017 17:16:45 +0800
>> > Jonas Ådahl <jadahl@gmail.com> wrote:
>> >
>> > > Two different protocols may use interfaces with identical names.
>> > > Implementing support for both those protocols would result in symbol
>> > > clashes, as wayland-scanner generates symbols from the interface names.
>> > >
>> > > Make it possible to avoiding these clashes by adding a way to add a
>> > > prefix to the symbols generated by wayland-scanner. Implementations
>> > > (servers and clients) can then use these prefix:ed symbols to implement
>> > > different objects with the same name.
>> > >
>> > > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
>> > > ---
>> > >
>> > > Something like this would be needed if a compositor/client wants to implement
>> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
>> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
>> > > xdg-shell stable does not have this issue.
>> > >
>> > > See issue raised here:
>> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
>> > >
>> > >
>> > > Jonas
>> > >
>> > >
>> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> > >  1 file changed, 73 insertions(+), 21 deletions(-)
>> >
>> >
>> > Hi,
>> >
>> > while this seems to change the ABI symbol names, it does not change the
>> > names in the documentation, and it does not change the names of
>> > #defines of enums, or the inline functions. That means that this is not
>> > enough to fulfill the purpose: being able to use two similarly named
>> > but different protocols by adding a prefix.
>>
>> The idea I had was rather that one would avoid changing any names on
>> non-symbols. It'd still be possible to implement both by doing it in
>> separate C files. I can see the point in adding the prefix on everything
>> though, so I'll provide a patch for that.
>>
>
> Hi,
>
> recapping the discussion from IRC, we pretty much agreed that prefixing
> is not a nice solution. Jonas found out that we cannot actually prefix
> everything, because there usually are references to other protocol
> things (like you would never want to prefix wl_surface). So it becomes
> very hard to prefix things appropriately.
>
> The alternative we discussed is solving a different problem: scanner
> makes all the public symbols in the generated .c files WL_EXPORT, which
> makes them leak into DSO ABI, which is bad. In my opinion, it should
> have never happened in the first place. But we missed it, and now it has
> spread, so we cannot just fix scanner to stop exporting, the decision
> must be with the consumers. So we need a scanner option to stop
> exporting.
>
> Quentin proposed we add a scanner option
> --visibility={static|compiler|export}. It would affect all the symbols
> exported from the generated .c files as follows:
>
> - static: the symbols will be static.
> - compiler: the symbols will get whatever the default visibility is
>   with the compiler, i.e. not explicitly static and not exported
> - export: the symbols are exported (this the old behaviour, and will be
>   the default)
>
> Obviously, the only way to actually make use of the 'static' option is
> for the consumer to #include the generated .c file. It's ugly, yes, but
> it solves the conflicting symbol names issue Jonas was looking into.
>
> In my opinion, the prefixing approach where we still cannot prefix
> everything in a way that one could use conflicting protocols in the
> same compilation unit, and where e.g. the static inline functions are
> not prefixed, is more ugly than the 'static' option.
>
> We are going to need an option to stop the exports anyway, and it seems
> like we can piggyback on that solution for the problem underlying the
> prefixing proposal as well.
>
It sounds like Quentin proposal is trying to address two distinct
things at the same time:
 - expose correct symbol visiblity
 - avoid symbol crashes due to conflicting naming used for "different" protocols

Former is addressed by swapping WL_EXPORT with WL_PRIVATE, or alike
While the latter can be tackled in a couple of says:
 - manually update the sources to use separate distinct name for the protocol
 - convince the generator (scanner) to produce ones for us

I think convincing the scanner is a perfectly reasonable solution.

HTH
Emil
On Wed, 26 Jul 2017 16:09:32 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Tue, 25 Jul 2017 15:25:58 +0800
> > Jonas Ådahl <jadahl@gmail.com> wrote:
> >  
> >> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:  
> >> > On Mon,  3 Jul 2017 17:16:45 +0800
> >> > Jonas Ådahl <jadahl@gmail.com> wrote:
> >> >  
> >> > > Two different protocols may use interfaces with identical names.
> >> > > Implementing support for both those protocols would result in symbol
> >> > > clashes, as wayland-scanner generates symbols from the interface names.
> >> > >
> >> > > Make it possible to avoiding these clashes by adding a way to add a
> >> > > prefix to the symbols generated by wayland-scanner. Implementations
> >> > > (servers and clients) can then use these prefix:ed symbols to implement
> >> > > different objects with the same name.
> >> > >
> >> > > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> >> > > ---
> >> > >
> >> > > Something like this would be needed if a compositor/client wants to implement
> >> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> >> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> >> > > xdg-shell stable does not have this issue.
> >> > >
> >> > > See issue raised here:
> >> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> >> > >
> >> > >
> >> > > Jonas
> >> > >
> >> > >
> >> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >> > >  1 file changed, 73 insertions(+), 21 deletions(-)  
> >> >
> >> >
> >> > Hi,
> >> >
> >> > while this seems to change the ABI symbol names, it does not change the
> >> > names in the documentation, and it does not change the names of
> >> > #defines of enums, or the inline functions. That means that this is not
> >> > enough to fulfill the purpose: being able to use two similarly named
> >> > but different protocols by adding a prefix.  
> >>
> >> The idea I had was rather that one would avoid changing any names on
> >> non-symbols. It'd still be possible to implement both by doing it in
> >> separate C files. I can see the point in adding the prefix on everything
> >> though, so I'll provide a patch for that.
> >>  
> >
> > Hi,
> >
> > recapping the discussion from IRC, we pretty much agreed that prefixing
> > is not a nice solution. Jonas found out that we cannot actually prefix
> > everything, because there usually are references to other protocol
> > things (like you would never want to prefix wl_surface). So it becomes
> > very hard to prefix things appropriately.
> >
> > The alternative we discussed is solving a different problem: scanner
> > makes all the public symbols in the generated .c files WL_EXPORT, which
> > makes them leak into DSO ABI, which is bad. In my opinion, it should
> > have never happened in the first place. But we missed it, and now it has
> > spread, so we cannot just fix scanner to stop exporting, the decision
> > must be with the consumers. So we need a scanner option to stop
> > exporting.
> >
> > Quentin proposed we add a scanner option
> > --visibility={static|compiler|export}. It would affect all the symbols
> > exported from the generated .c files as follows:
> >
> > - static: the symbols will be static.
> > - compiler: the symbols will get whatever the default visibility is
> >   with the compiler, i.e. not explicitly static and not exported
> > - export: the symbols are exported (this the old behaviour, and will be
> >   the default)
> >
> > Obviously, the only way to actually make use of the 'static' option is
> > for the consumer to #include the generated .c file. It's ugly, yes, but
> > it solves the conflicting symbol names issue Jonas was looking into.
> >
> > In my opinion, the prefixing approach where we still cannot prefix
> > everything in a way that one could use conflicting protocols in the
> > same compilation unit, and where e.g. the static inline functions are
> > not prefixed, is more ugly than the 'static' option.
> >
> > We are going to need an option to stop the exports anyway, and it seems
> > like we can piggyback on that solution for the problem underlying the
> > prefixing proposal as well.
> >  
> It sounds like Quentin proposal is trying to address two distinct
> things at the same time:
>  - expose correct symbol visiblity
>  - avoid symbol crashes due to conflicting naming used for "different" protocols

Yes, it indeed is.

> Former is addressed by swapping WL_EXPORT with WL_PRIVATE, or alike
> While the latter can be tackled in a couple of says:
>  - manually update the sources to use separate distinct name for the protocol
>  - convince the generator (scanner) to produce ones for us
> 
> I think convincing the scanner is a perfectly reasonable solution.

Does that mean you are in favour of this patch 2/3 as is?
Is it ok to prefix only the "ABI symbols" and leave everything else in
the API, e.g. static inline functions, unprefixed?


Thanks,
pq
On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Wed, 26 Jul 2017 16:09:32 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>> > On Tue, 25 Jul 2017 15:25:58 +0800
>> > Jonas Ådahl <jadahl@gmail.com> wrote:
>> >
>> >> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
>> >> > On Mon,  3 Jul 2017 17:16:45 +0800
>> >> > Jonas Ådahl <jadahl@gmail.com> wrote:
>> >> >
>> >> > > Two different protocols may use interfaces with identical names.
>> >> > > Implementing support for both those protocols would result in symbol
>> >> > > clashes, as wayland-scanner generates symbols from the interface names.
>> >> > >
>> >> > > Make it possible to avoiding these clashes by adding a way to add a
>> >> > > prefix to the symbols generated by wayland-scanner. Implementations
>> >> > > (servers and clients) can then use these prefix:ed symbols to implement
>> >> > > different objects with the same name.
>> >> > >
>> >> > > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
>> >> > > ---
>> >> > >
>> >> > > Something like this would be needed if a compositor/client wants to implement
>> >> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
>> >> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
>> >> > > xdg-shell stable does not have this issue.
>> >> > >
>> >> > > See issue raised here:
>> >> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
>> >> > >
>> >> > >
>> >> > > Jonas
>> >> > >
>> >> > >
>> >> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>> >> > >  1 file changed, 73 insertions(+), 21 deletions(-)
>> >> >
>> >> >
>> >> > Hi,
>> >> >
>> >> > while this seems to change the ABI symbol names, it does not change the
>> >> > names in the documentation, and it does not change the names of
>> >> > #defines of enums, or the inline functions. That means that this is not
>> >> > enough to fulfill the purpose: being able to use two similarly named
>> >> > but different protocols by adding a prefix.
>> >>
>> >> The idea I had was rather that one would avoid changing any names on
>> >> non-symbols. It'd still be possible to implement both by doing it in
>> >> separate C files. I can see the point in adding the prefix on everything
>> >> though, so I'll provide a patch for that.
>> >>
>> >
>> > Hi,
>> >
>> > recapping the discussion from IRC, we pretty much agreed that prefixing
>> > is not a nice solution. Jonas found out that we cannot actually prefix
>> > everything, because there usually are references to other protocol
>> > things (like you would never want to prefix wl_surface). So it becomes
>> > very hard to prefix things appropriately.
>> >
>> > The alternative we discussed is solving a different problem: scanner
>> > makes all the public symbols in the generated .c files WL_EXPORT, which
>> > makes them leak into DSO ABI, which is bad. In my opinion, it should
>> > have never happened in the first place. But we missed it, and now it has
>> > spread, so we cannot just fix scanner to stop exporting, the decision
>> > must be with the consumers. So we need a scanner option to stop
>> > exporting.
>> >
>> > Quentin proposed we add a scanner option
>> > --visibility={static|compiler|export}. It would affect all the symbols
>> > exported from the generated .c files as follows:
>> >
>> > - static: the symbols will be static.
>> > - compiler: the symbols will get whatever the default visibility is
>> >   with the compiler, i.e. not explicitly static and not exported
>> > - export: the symbols are exported (this the old behaviour, and will be
>> >   the default)
>> >
>> > Obviously, the only way to actually make use of the 'static' option is
>> > for the consumer to #include the generated .c file. It's ugly, yes, but
>> > it solves the conflicting symbol names issue Jonas was looking into.
>> >
>> > In my opinion, the prefixing approach where we still cannot prefix
>> > everything in a way that one could use conflicting protocols in the
>> > same compilation unit, and where e.g. the static inline functions are
>> > not prefixed, is more ugly than the 'static' option.
>> >
>> > We are going to need an option to stop the exports anyway, and it seems
>> > like we can piggyback on that solution for the problem underlying the
>> > prefixing proposal as well.
>> >
>> It sounds like Quentin proposal is trying to address two distinct
>> things at the same time:
>>  - expose correct symbol visiblity
>>  - avoid symbol crashes due to conflicting naming used for "different" protocols
>
> Yes, it indeed is.
>
>> Former is addressed by swapping WL_EXPORT with WL_PRIVATE, or alike
>> While the latter can be tackled in a couple of says:
>>  - manually update the sources to use separate distinct name for the protocol
>>  - convince the generator (scanner) to produce ones for us
>>
>> I think convincing the scanner is a perfectly reasonable solution.
>
> Does that mean you are in favour of this patch 2/3 as is?
> Is it ok to prefix only the "ABI symbols" and leave everything else in
> the API, e.g. static inline functions, unprefixed?
>

Tl:Dr; I think that everything should be prefixed. See below for details.

 - ABI symbols - to prevent symbol collision
 - inline/other API - to minimise ambiguity, confusion, plus you can
have multiple implementations in the same translation unit*


*Some projects may want to support xdg-shell vX and vX+1 in the same file.
 - The inline API from vX may work with vY or vise-versa. Yet it's
easier to pass the wrong object to the inline function.
 - One can (and will have) cases, where API foo() is available for
only one version.
 - If the function signature (of the inline API) differs the compiler
will barf at you - say vX+1 adds extra argument to function bar()

Thanks
Emil
On Fri, 28 Jul 2017 14:06:23 +0100
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > On Wed, 26 Jul 2017 16:09:32 +0100
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >  
> >> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> >> > On Tue, 25 Jul 2017 15:25:58 +0800
> >> > Jonas Ådahl <jadahl@gmail.com> wrote:
> >> >  
> >> >> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:  
> >> >> > On Mon,  3 Jul 2017 17:16:45 +0800
> >> >> > Jonas Ådahl <jadahl@gmail.com> wrote:
> >> >> >  
> >> >> > > Two different protocols may use interfaces with identical names.
> >> >> > > Implementing support for both those protocols would result in symbol
> >> >> > > clashes, as wayland-scanner generates symbols from the interface names.
> >> >> > >
> >> >> > > Make it possible to avoiding these clashes by adding a way to add a
> >> >> > > prefix to the symbols generated by wayland-scanner. Implementations
> >> >> > > (servers and clients) can then use these prefix:ed symbols to implement
> >> >> > > different objects with the same name.
> >> >> > >
> >> >> > > Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> >> >> > > ---
> >> >> > >
> >> >> > > Something like this would be needed if a compositor/client wants to implement
> >> >> > > xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> >> >> > > our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> >> >> > > xdg-shell stable does not have this issue.
> >> >> > >
> >> >> > > See issue raised here:
> >> >> > > https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> >> >> > >
> >> >> > >
> >> >> > > Jonas
> >> >> > >
> >> >> > >
> >> >> > >  src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >> >> > >  1 file changed, 73 insertions(+), 21 deletions(-)  
> >> >> >
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > while this seems to change the ABI symbol names, it does not change the
> >> >> > names in the documentation, and it does not change the names of
> >> >> > #defines of enums, or the inline functions. That means that this is not
> >> >> > enough to fulfill the purpose: being able to use two similarly named
> >> >> > but different protocols by adding a prefix.  
> >> >>
> >> >> The idea I had was rather that one would avoid changing any names on
> >> >> non-symbols. It'd still be possible to implement both by doing it in
> >> >> separate C files. I can see the point in adding the prefix on everything
> >> >> though, so I'll provide a patch for that.
> >> >>  
> >> >
> >> > Hi,
> >> >
> >> > recapping the discussion from IRC, we pretty much agreed that prefixing
> >> > is not a nice solution. Jonas found out that we cannot actually prefix
> >> > everything, because there usually are references to other protocol
> >> > things (like you would never want to prefix wl_surface). So it becomes
> >> > very hard to prefix things appropriately.

...

> Tl:Dr; I think that everything should be prefixed. See below for details.
> 
>  - ABI symbols - to prevent symbol collision
>  - inline/other API - to minimise ambiguity, confusion, plus you can
> have multiple implementations in the same translation unit*
> 
> 
> *Some projects may want to support xdg-shell vX and vX+1 in the same file.
>  - The inline API from vX may work with vY or vise-versa. Yet it's
> easier to pass the wrong object to the inline function.
>  - One can (and will have) cases, where API foo() is available for
> only one version.
>  - If the function signature (of the inline API) differs the compiler
> will barf at you - say vX+1 adds extra argument to function bar()

Yes, I agree, which is why I was not too fond of this patch originally.
This patch does not prefix API, but only ABI.

The difficulty is in what interfaces to prefix as a whole (ABI and API)
and what not. As an example, one would never want to prefix wl_surface,
yet extension interfaces use wl_surface as a message argument.

Jonas, I think we can discriminate between interfaces (protocol object
types) defined in the XML file being processed vs. external interfaces
from elsewhere (like wl_surface). Would that be good enough for
prefixing not just #defines and inline functions but the C types as
well?

The case where it would not work if one wants to prefix things from
both one.xml and two.xml, and two.xml used interfaces defined in
one.xml. Is that an acceptable limitation?

As a stretch I'd rather avoid for now, there could be an option to
prefix all external interfaces with another prefix, but it would not be
able to make a difference between things from wayland.xml and one.xml,
for instance.

Where are we at with the name collision problem in general now?


Thanks,
pq
On 2017-08-18 07:41 AM, Pekka Paalanen wrote:
> On Fri, 28 Jul 2017 14:06:23 +0100
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> 
>> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>> On Wed, 26 Jul 2017 16:09:32 +0100
>>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>   
>>>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>> On Tue, 25 Jul 2017 15:25:58 +0800
>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>   
>>>>>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
>>>>>>> On Mon,  3 Jul 2017 17:16:45 +0800
>>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>>>   
>>>>>>>> Two different protocols may use interfaces with identical names.
>>>>>>>> Implementing support for both those protocols would result in symbol
>>>>>>>> clashes, as wayland-scanner generates symbols from the interface names.
>>>>>>>>
>>>>>>>> Make it possible to avoiding these clashes by adding a way to add a
>>>>>>>> prefix to the symbols generated by wayland-scanner. Implementations
>>>>>>>> (servers and clients) can then use these prefix:ed symbols to implement
>>>>>>>> different objects with the same name.
>>>>>>>>
>>>>>>>> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Something like this would be needed if a compositor/client wants to implement
>>>>>>>> xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
>>>>>>>> our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
>>>>>>>> xdg-shell stable does not have this issue.
>>>>>>>>
>>>>>>>> See issue raised here:
>>>>>>>> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
>>>>>>>>
>>>>>>>>
>>>>>>>> Jonas
>>>>>>>>
>>>>>>>>
>>>>>>>>   src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>>>>>>>   1 file changed, 73 insertions(+), 21 deletions(-)
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> while this seems to change the ABI symbol names, it does not change the
>>>>>>> names in the documentation, and it does not change the names of
>>>>>>> #defines of enums, or the inline functions. That means that this is not
>>>>>>> enough to fulfill the purpose: being able to use two similarly named
>>>>>>> but different protocols by adding a prefix.
>>>>>>
>>>>>> The idea I had was rather that one would avoid changing any names on
>>>>>> non-symbols. It'd still be possible to implement both by doing it in
>>>>>> separate C files. I can see the point in adding the prefix on everything
>>>>>> though, so I'll provide a patch for that.
>>>>>>   
>>>>>
>>>>> Hi,
>>>>>
>>>>> recapping the discussion from IRC, we pretty much agreed that prefixing
>>>>> is not a nice solution. Jonas found out that we cannot actually prefix
>>>>> everything, because there usually are references to other protocol
>>>>> things (like you would never want to prefix wl_surface). So it becomes
>>>>> very hard to prefix things appropriately.
> 
> ...
> 
>> Tl:Dr; I think that everything should be prefixed. See below for details.
>>
>>   - ABI symbols - to prevent symbol collision
>>   - inline/other API - to minimise ambiguity, confusion, plus you can
>> have multiple implementations in the same translation unit*
>>
>>
>> *Some projects may want to support xdg-shell vX and vX+1 in the same file.
>>   - The inline API from vX may work with vY or vise-versa. Yet it's
>> easier to pass the wrong object to the inline function.
>>   - One can (and will have) cases, where API foo() is available for
>> only one version.
>>   - If the function signature (of the inline API) differs the compiler
>> will barf at you - say vX+1 adds extra argument to function bar()
> 
> Yes, I agree, which is why I was not too fond of this patch originally.
> This patch does not prefix API, but only ABI.
> 
> The difficulty is in what interfaces to prefix as a whole (ABI and API)
> and what not. As an example, one would never want to prefix wl_surface,
> yet extension interfaces use wl_surface as a message argument.
> 
> Jonas, I think we can discriminate between interfaces (protocol object
> types) defined in the XML file being processed vs. external interfaces
> from elsewhere (like wl_surface). Would that be good enough for
> prefixing not just #defines and inline functions but the C types as
> well?
> 
> The case where it would not work if one wants to prefix things from
> both one.xml and two.xml, and two.xml used interfaces defined in
> one.xml. Is that an acceptable limitation?
> 
> As a stretch I'd rather avoid for now, there could be an option to
> prefix all external interfaces with another prefix, but it would not be
> able to make a difference between things from wayland.xml and one.xml,
> for instance.

I think I'd like to avoid that too...

> Where are we at with the name collision problem in general now?

Bit of a necropost, but was any kind of consensus in reach here? :)

Emil's nominated this for inclusion in an upcoming release, and it would 
certainly get us out of the xdg-shell-v5 vs xdg-shell-stable collisions 
we face now (if we intend to have weston support xdg-shell-stable 
without dropping v5 at some point).

Does this have a strong use case outside of just dealing with the v5 vs 
stable thing?  We could probably solve that with a sed script (in 
wayland-protocols?)

Thanks,
Derek

> 
> Thanks,
> pq
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On Wed, 17 Jan 2018 15:43:07 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 2017-08-18 07:41 AM, Pekka Paalanen wrote:
> > On Fri, 28 Jul 2017 14:06:23 +0100
> > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >   
> >> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> >>> On Wed, 26 Jul 2017 16:09:32 +0100
> >>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >>>     
> >>>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> >>>>> On Tue, 25 Jul 2017 15:25:58 +0800
> >>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
> >>>>>     
> >>>>>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:  
> >>>>>>> On Mon,  3 Jul 2017 17:16:45 +0800
> >>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
> >>>>>>>     
> >>>>>>>> Two different protocols may use interfaces with identical names.
> >>>>>>>> Implementing support for both those protocols would result in symbol
> >>>>>>>> clashes, as wayland-scanner generates symbols from the interface names.
> >>>>>>>>
> >>>>>>>> Make it possible to avoiding these clashes by adding a way to add a
> >>>>>>>> prefix to the symbols generated by wayland-scanner. Implementations
> >>>>>>>> (servers and clients) can then use these prefix:ed symbols to implement
> >>>>>>>> different objects with the same name.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> Something like this would be needed if a compositor/client wants to implement
> >>>>>>>> xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> >>>>>>>> our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> >>>>>>>> xdg-shell stable does not have this issue.
> >>>>>>>>
> >>>>>>>> See issue raised here:
> >>>>>>>> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Jonas
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>   src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> >>>>>>>>   1 file changed, 73 insertions(+), 21 deletions(-)  
> >>>>>>>
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> while this seems to change the ABI symbol names, it does not change the
> >>>>>>> names in the documentation, and it does not change the names of
> >>>>>>> #defines of enums, or the inline functions. That means that this is not
> >>>>>>> enough to fulfill the purpose: being able to use two similarly named
> >>>>>>> but different protocols by adding a prefix.  
> >>>>>>
> >>>>>> The idea I had was rather that one would avoid changing any names on
> >>>>>> non-symbols. It'd still be possible to implement both by doing it in
> >>>>>> separate C files. I can see the point in adding the prefix on everything
> >>>>>> though, so I'll provide a patch for that.
> >>>>>>     
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> recapping the discussion from IRC, we pretty much agreed that prefixing
> >>>>> is not a nice solution. Jonas found out that we cannot actually prefix
> >>>>> everything, because there usually are references to other protocol
> >>>>> things (like you would never want to prefix wl_surface). So it becomes
> >>>>> very hard to prefix things appropriately.  
> > 
> > ...
> >   
> >> Tl:Dr; I think that everything should be prefixed. See below for details.
> >>
> >>   - ABI symbols - to prevent symbol collision
> >>   - inline/other API - to minimise ambiguity, confusion, plus you can
> >> have multiple implementations in the same translation unit*
> >>
> >>
> >> *Some projects may want to support xdg-shell vX and vX+1 in the same file.
> >>   - The inline API from vX may work with vY or vise-versa. Yet it's
> >> easier to pass the wrong object to the inline function.
> >>   - One can (and will have) cases, where API foo() is available for
> >> only one version.
> >>   - If the function signature (of the inline API) differs the compiler
> >> will barf at you - say vX+1 adds extra argument to function bar()  
> > 
> > Yes, I agree, which is why I was not too fond of this patch originally.
> > This patch does not prefix API, but only ABI.
> > 
> > The difficulty is in what interfaces to prefix as a whole (ABI and API)
> > and what not. As an example, one would never want to prefix wl_surface,
> > yet extension interfaces use wl_surface as a message argument.
> > 
> > Jonas, I think we can discriminate between interfaces (protocol object
> > types) defined in the XML file being processed vs. external interfaces
> > from elsewhere (like wl_surface). Would that be good enough for
> > prefixing not just #defines and inline functions but the C types as
> > well?
> > 
> > The case where it would not work if one wants to prefix things from
> > both one.xml and two.xml, and two.xml used interfaces defined in
> > one.xml. Is that an acceptable limitation?
> > 
> > As a stretch I'd rather avoid for now, there could be an option to
> > prefix all external interfaces with another prefix, but it would not be
> > able to make a difference between things from wayland.xml and one.xml,
> > for instance.  
> 
> I think I'd like to avoid that too...
> 
> > Where are we at with the name collision problem in general now?  
> 
> Bit of a necropost, but was any kind of consensus in reach here? :)
> 
> Emil's nominated this for inclusion in an upcoming release, and it would 
> certainly get us out of the xdg-shell-v5 vs xdg-shell-stable collisions 
> we face now (if we intend to have weston support xdg-shell-stable 
> without dropping v5 at some point).
> 
> Does this have a strong use case outside of just dealing with the v5 vs 
> stable thing?  We could probably solve that with a sed script (in 
> wayland-protocols?)

Hi,

the original proposal was to prefix ABI symbols, and leave everything
else as is. Maybe writing down again how exactly that is supposed to be
used and what problems it solves would crystallize the idea. Perhaps in
the form of wayland-scanner user instructions which the original patch
seems to be lacking?

Earlier I didn't like prefixing only some bits of the API, but on
second thought, if that's all what's needed, maybe it isn't that bad
from hand-written code readability point of view?

The discussion showed that any other solution becomes hard and/or messy
compared to that. This is not a library ABI/API change either, just a
new operation mode in wayland-scanner.

I join you with the question about use cases and how badly do we need
this.


Thanks,
pq
On Thu, Jan 18, 2018 at 10:48:14AM +0200, Pekka Paalanen wrote:
> On Wed, 17 Jan 2018 15:43:07 -0600
> Derek Foreman <derekf@osg.samsung.com> wrote:
> 
> > On 2017-08-18 07:41 AM, Pekka Paalanen wrote:
> > > On Fri, 28 Jul 2017 14:06:23 +0100
> > > Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >   
> > >> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > >>> On Wed, 26 Jul 2017 16:09:32 +0100
> > >>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
> > >>>     
> > >>>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > >>>>> On Tue, 25 Jul 2017 15:25:58 +0800
> > >>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
> > >>>>>     
> > >>>>>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:  
> > >>>>>>> On Mon,  3 Jul 2017 17:16:45 +0800
> > >>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
> > >>>>>>>     
> > >>>>>>>> Two different protocols may use interfaces with identical names.
> > >>>>>>>> Implementing support for both those protocols would result in symbol
> > >>>>>>>> clashes, as wayland-scanner generates symbols from the interface names.
> > >>>>>>>>
> > >>>>>>>> Make it possible to avoiding these clashes by adding a way to add a
> > >>>>>>>> prefix to the symbols generated by wayland-scanner. Implementations
> > >>>>>>>> (servers and clients) can then use these prefix:ed symbols to implement
> > >>>>>>>> different objects with the same name.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
> > >>>>>>>> ---
> > >>>>>>>>
> > >>>>>>>> Something like this would be needed if a compositor/client wants to implement
> > >>>>>>>> xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
> > >>>>>>>> our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
> > >>>>>>>> xdg-shell stable does not have this issue.
> > >>>>>>>>
> > >>>>>>>> See issue raised here:
> > >>>>>>>> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> Jonas
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>   src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
> > >>>>>>>>   1 file changed, 73 insertions(+), 21 deletions(-)  
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> while this seems to change the ABI symbol names, it does not change the
> > >>>>>>> names in the documentation, and it does not change the names of
> > >>>>>>> #defines of enums, or the inline functions. That means that this is not
> > >>>>>>> enough to fulfill the purpose: being able to use two similarly named
> > >>>>>>> but different protocols by adding a prefix.  
> > >>>>>>
> > >>>>>> The idea I had was rather that one would avoid changing any names on
> > >>>>>> non-symbols. It'd still be possible to implement both by doing it in
> > >>>>>> separate C files. I can see the point in adding the prefix on everything
> > >>>>>> though, so I'll provide a patch for that.
> > >>>>>>     
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> recapping the discussion from IRC, we pretty much agreed that prefixing
> > >>>>> is not a nice solution. Jonas found out that we cannot actually prefix
> > >>>>> everything, because there usually are references to other protocol
> > >>>>> things (like you would never want to prefix wl_surface). So it becomes
> > >>>>> very hard to prefix things appropriately.  
> > > 
> > > ...
> > >   
> > >> Tl:Dr; I think that everything should be prefixed. See below for details.
> > >>
> > >>   - ABI symbols - to prevent symbol collision
> > >>   - inline/other API - to minimise ambiguity, confusion, plus you can
> > >> have multiple implementations in the same translation unit*
> > >>
> > >>
> > >> *Some projects may want to support xdg-shell vX and vX+1 in the same file.
> > >>   - The inline API from vX may work with vY or vise-versa. Yet it's
> > >> easier to pass the wrong object to the inline function.
> > >>   - One can (and will have) cases, where API foo() is available for
> > >> only one version.
> > >>   - If the function signature (of the inline API) differs the compiler
> > >> will barf at you - say vX+1 adds extra argument to function bar()  
> > > 
> > > Yes, I agree, which is why I was not too fond of this patch originally.
> > > This patch does not prefix API, but only ABI.
> > > 
> > > The difficulty is in what interfaces to prefix as a whole (ABI and API)
> > > and what not. As an example, one would never want to prefix wl_surface,
> > > yet extension interfaces use wl_surface as a message argument.
> > > 
> > > Jonas, I think we can discriminate between interfaces (protocol object
> > > types) defined in the XML file being processed vs. external interfaces
> > > from elsewhere (like wl_surface). Would that be good enough for
> > > prefixing not just #defines and inline functions but the C types as
> > > well?
> > > 
> > > The case where it would not work if one wants to prefix things from
> > > both one.xml and two.xml, and two.xml used interfaces defined in
> > > one.xml. Is that an acceptable limitation?
> > > 
> > > As a stretch I'd rather avoid for now, there could be an option to
> > > prefix all external interfaces with another prefix, but it would not be
> > > able to make a difference between things from wayland.xml and one.xml,
> > > for instance.  
> > 
> > I think I'd like to avoid that too...
> > 
> > > Where are we at with the name collision problem in general now?  
> > 
> > Bit of a necropost, but was any kind of consensus in reach here? :)
> > 
> > Emil's nominated this for inclusion in an upcoming release, and it would 
> > certainly get us out of the xdg-shell-v5 vs xdg-shell-stable collisions 
> > we face now (if we intend to have weston support xdg-shell-stable 
> > without dropping v5 at some point).
> > 
> > Does this have a strong use case outside of just dealing with the v5 vs 
> > stable thing?  We could probably solve that with a sed script (in 
> > wayland-protocols?)
> 
> Hi,
> 
> the original proposal was to prefix ABI symbols, and leave everything
> else as is. Maybe writing down again how exactly that is supposed to be
> used and what problems it solves would crystallize the idea. Perhaps in
> the form of wayland-scanner user instructions which the original patch
> seems to be lacking?
> 
> Earlier I didn't like prefixing only some bits of the API, but on
> second thought, if that's all what's needed, maybe it isn't that bad
> from hand-written code readability point of view?
> 
> The discussion showed that any other solution becomes hard and/or messy
> compared to that. This is not a library ABI/API change either, just a
> new operation mode in wayland-scanner.
> 
> I join you with the question about use cases and how badly do we need
> this.

I don't see the point in keeping xdg-shell unstable v5 support in
weston, so from that point of view, *weston* I'd say don't need any
symbol prefix feature.


Jonas

> 
> 
> Thanks,
> pq
On 2018-01-19 01:22 AM, Jonas Ådahl wrote:
> On Thu, Jan 18, 2018 at 10:48:14AM +0200, Pekka Paalanen wrote:
>> On Wed, 17 Jan 2018 15:43:07 -0600
>> Derek Foreman <derekf@osg.samsung.com> wrote:
>>
>>> On 2017-08-18 07:41 AM, Pekka Paalanen wrote:
>>>> On Fri, 28 Jul 2017 14:06:23 +0100
>>>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>    
>>>>> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>>> On Wed, 26 Jul 2017 16:09:32 +0100
>>>>>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>>      
>>>>>>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>>>>> On Tue, 25 Jul 2017 15:25:58 +0800
>>>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>>>>      
>>>>>>>>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
>>>>>>>>>> On Mon,  3 Jul 2017 17:16:45 +0800
>>>>>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>>>>>>      
>>>>>>>>>>> Two different protocols may use interfaces with identical names.
>>>>>>>>>>> Implementing support for both those protocols would result in symbol
>>>>>>>>>>> clashes, as wayland-scanner generates symbols from the interface names.
>>>>>>>>>>>
>>>>>>>>>>> Make it possible to avoiding these clashes by adding a way to add a
>>>>>>>>>>> prefix to the symbols generated by wayland-scanner. Implementations
>>>>>>>>>>> (servers and clients) can then use these prefix:ed symbols to implement
>>>>>>>>>>> different objects with the same name.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Something like this would be needed if a compositor/client wants to implement
>>>>>>>>>>> xdg-shell unstable v5 alongside xdg-shell stable, unless we want to rename all
>>>>>>>>>>> our xdg-shell interfaces. Implementing xdg-shell unstable v6 alongside
>>>>>>>>>>> xdg-shell stable does not have this issue.
>>>>>>>>>>>
>>>>>>>>>>> See issue raised here:
>>>>>>>>>>> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Jonas
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>    src/scanner.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>>>>>>>>>>    1 file changed, 73 insertions(+), 21 deletions(-)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> while this seems to change the ABI symbol names, it does not change the
>>>>>>>>>> names in the documentation, and it does not change the names of
>>>>>>>>>> #defines of enums, or the inline functions. That means that this is not
>>>>>>>>>> enough to fulfill the purpose: being able to use two similarly named
>>>>>>>>>> but different protocols by adding a prefix.
>>>>>>>>>
>>>>>>>>> The idea I had was rather that one would avoid changing any names on
>>>>>>>>> non-symbols. It'd still be possible to implement both by doing it in
>>>>>>>>> separate C files. I can see the point in adding the prefix on everything
>>>>>>>>> though, so I'll provide a patch for that.
>>>>>>>>>      
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> recapping the discussion from IRC, we pretty much agreed that prefixing
>>>>>>>> is not a nice solution. Jonas found out that we cannot actually prefix
>>>>>>>> everything, because there usually are references to other protocol
>>>>>>>> things (like you would never want to prefix wl_surface). So it becomes
>>>>>>>> very hard to prefix things appropriately.
>>>>
>>>> ...
>>>>    
>>>>> Tl:Dr; I think that everything should be prefixed. See below for details.
>>>>>
>>>>>    - ABI symbols - to prevent symbol collision
>>>>>    - inline/other API - to minimise ambiguity, confusion, plus you can
>>>>> have multiple implementations in the same translation unit*
>>>>>
>>>>>
>>>>> *Some projects may want to support xdg-shell vX and vX+1 in the same file.
>>>>>    - The inline API from vX may work with vY or vise-versa. Yet it's
>>>>> easier to pass the wrong object to the inline function.
>>>>>    - One can (and will have) cases, where API foo() is available for
>>>>> only one version.
>>>>>    - If the function signature (of the inline API) differs the compiler
>>>>> will barf at you - say vX+1 adds extra argument to function bar()
>>>>
>>>> Yes, I agree, which is why I was not too fond of this patch originally.
>>>> This patch does not prefix API, but only ABI.
>>>>
>>>> The difficulty is in what interfaces to prefix as a whole (ABI and API)
>>>> and what not. As an example, one would never want to prefix wl_surface,
>>>> yet extension interfaces use wl_surface as a message argument.
>>>>
>>>> Jonas, I think we can discriminate between interfaces (protocol object
>>>> types) defined in the XML file being processed vs. external interfaces
>>>> from elsewhere (like wl_surface). Would that be good enough for
>>>> prefixing not just #defines and inline functions but the C types as
>>>> well?
>>>>
>>>> The case where it would not work if one wants to prefix things from
>>>> both one.xml and two.xml, and two.xml used interfaces defined in
>>>> one.xml. Is that an acceptable limitation?
>>>>
>>>> As a stretch I'd rather avoid for now, there could be an option to
>>>> prefix all external interfaces with another prefix, but it would not be
>>>> able to make a difference between things from wayland.xml and one.xml,
>>>> for instance.
>>>
>>> I think I'd like to avoid that too...
>>>
>>>> Where are we at with the name collision problem in general now?
>>>
>>> Bit of a necropost, but was any kind of consensus in reach here? :)
>>>
>>> Emil's nominated this for inclusion in an upcoming release, and it would
>>> certainly get us out of the xdg-shell-v5 vs xdg-shell-stable collisions
>>> we face now (if we intend to have weston support xdg-shell-stable
>>> without dropping v5 at some point).
>>>
>>> Does this have a strong use case outside of just dealing with the v5 vs
>>> stable thing?  We could probably solve that with a sed script (in
>>> wayland-protocols?)
>>
>> Hi,
>>
>> the original proposal was to prefix ABI symbols, and leave everything
>> else as is. Maybe writing down again how exactly that is supposed to be
>> used and what problems it solves would crystallize the idea. Perhaps in
>> the form of wayland-scanner user instructions which the original patch
>> seems to be lacking?
>>
>> Earlier I didn't like prefixing only some bits of the API, but on
>> second thought, if that's all what's needed, maybe it isn't that bad
>> from hand-written code readability point of view?
>>
>> The discussion showed that any other solution becomes hard and/or messy
>> compared to that. This is not a library ABI/API change either, just a
>> new operation mode in wayland-scanner.
>>
>> I join you with the question about use cases and how badly do we need
>> this.
> 
> I don't see the point in keeping xdg-shell unstable v5 support in
> weston, so from that point of view, *weston* I'd say don't need any
> symbol prefix feature.
> 

Ok, I guess at this point we should drop it from consideration for the 
next release.

Pekka's synopsis is a really good status report for anyone interested in 
picking this up later without reading all the previous threads.

Thanks all,
Derek

> Jonas
> 
>>
>>
>> Thanks,
>> pq
> 
>
On 19 January 2018 at 15:49, Derek Foreman <derekf@osg.samsung.com> wrote:
> On 2018-01-19 01:22 AM, Jonas Ådahl wrote:
>>
>> On Thu, Jan 18, 2018 at 10:48:14AM +0200, Pekka Paalanen wrote:
>>>
>>> On Wed, 17 Jan 2018 15:43:07 -0600
>>> Derek Foreman <derekf@osg.samsung.com> wrote:
>>>
>>>> On 2017-08-18 07:41 AM, Pekka Paalanen wrote:
>>>>>
>>>>> On Fri, 28 Jul 2017 14:06:23 +0100
>>>>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>
>>>>>>
>>>>>> On 27 July 2017 at 14:01, Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>>>>
>>>>>>> On Wed, 26 Jul 2017 16:09:32 +0100
>>>>>>> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On 25 July 2017 at 10:24, Pekka Paalanen <ppaalanen@gmail.com>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, 25 Jul 2017 15:25:58 +0800
>>>>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Mon, Jul 24, 2017 at 02:16:04PM +0300, Pekka Paalanen wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Mon,  3 Jul 2017 17:16:45 +0800
>>>>>>>>>>> Jonas Ådahl <jadahl@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Two different protocols may use interfaces with identical names.
>>>>>>>>>>>> Implementing support for both those protocols would result in
>>>>>>>>>>>> symbol
>>>>>>>>>>>> clashes, as wayland-scanner generates symbols from the interface
>>>>>>>>>>>> names.
>>>>>>>>>>>>
>>>>>>>>>>>> Make it possible to avoiding these clashes by adding a way to
>>>>>>>>>>>> add a
>>>>>>>>>>>> prefix to the symbols generated by wayland-scanner.
>>>>>>>>>>>> Implementations
>>>>>>>>>>>> (servers and clients) can then use these prefix:ed symbols to
>>>>>>>>>>>> implement
>>>>>>>>>>>> different objects with the same name.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> Something like this would be needed if a compositor/client wants
>>>>>>>>>>>> to implement
>>>>>>>>>>>> xdg-shell unstable v5 alongside xdg-shell stable, unless we want
>>>>>>>>>>>> to rename all
>>>>>>>>>>>> our xdg-shell interfaces. Implementing xdg-shell unstable v6
>>>>>>>>>>>> alongside
>>>>>>>>>>>> xdg-shell stable does not have this issue.
>>>>>>>>>>>>
>>>>>>>>>>>> See issue raised here:
>>>>>>>>>>>>
>>>>>>>>>>>> https://lists.freedesktop.org/archives/wayland-devel/2017-June/034380.html
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Jonas
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>    src/scanner.c | 94
>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>>>>>>>>>>>    1 file changed, 73 insertions(+), 21 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> while this seems to change the ABI symbol names, it does not
>>>>>>>>>>> change the
>>>>>>>>>>> names in the documentation, and it does not change the names of
>>>>>>>>>>> #defines of enums, or the inline functions. That means that this
>>>>>>>>>>> is not
>>>>>>>>>>> enough to fulfill the purpose: being able to use two similarly
>>>>>>>>>>> named
>>>>>>>>>>> but different protocols by adding a prefix.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The idea I had was rather that one would avoid changing any names
>>>>>>>>>> on
>>>>>>>>>> non-symbols. It'd still be possible to implement both by doing it
>>>>>>>>>> in
>>>>>>>>>> separate C files. I can see the point in adding the prefix on
>>>>>>>>>> everything
>>>>>>>>>> though, so I'll provide a patch for that.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> recapping the discussion from IRC, we pretty much agreed that
>>>>>>>>> prefixing
>>>>>>>>> is not a nice solution. Jonas found out that we cannot actually
>>>>>>>>> prefix
>>>>>>>>> everything, because there usually are references to other protocol
>>>>>>>>> things (like you would never want to prefix wl_surface). So it
>>>>>>>>> becomes
>>>>>>>>> very hard to prefix things appropriately.
>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>>
>>>>>> Tl:Dr; I think that everything should be prefixed. See below for
>>>>>> details.
>>>>>>
>>>>>>    - ABI symbols - to prevent symbol collision
>>>>>>    - inline/other API - to minimise ambiguity, confusion, plus you can
>>>>>> have multiple implementations in the same translation unit*
>>>>>>
>>>>>>
>>>>>> *Some projects may want to support xdg-shell vX and vX+1 in the same
>>>>>> file.
>>>>>>    - The inline API from vX may work with vY or vise-versa. Yet it's
>>>>>> easier to pass the wrong object to the inline function.
>>>>>>    - One can (and will have) cases, where API foo() is available for
>>>>>> only one version.
>>>>>>    - If the function signature (of the inline API) differs the
>>>>>> compiler
>>>>>> will barf at you - say vX+1 adds extra argument to function bar()
>>>>>
>>>>>
>>>>> Yes, I agree, which is why I was not too fond of this patch originally.
>>>>> This patch does not prefix API, but only ABI.
>>>>>
>>>>> The difficulty is in what interfaces to prefix as a whole (ABI and API)
>>>>> and what not. As an example, one would never want to prefix wl_surface,
>>>>> yet extension interfaces use wl_surface as a message argument.
>>>>>
>>>>> Jonas, I think we can discriminate between interfaces (protocol object
>>>>> types) defined in the XML file being processed vs. external interfaces
>>>>> from elsewhere (like wl_surface). Would that be good enough for
>>>>> prefixing not just #defines and inline functions but the C types as
>>>>> well?
>>>>>
>>>>> The case where it would not work if one wants to prefix things from
>>>>> both one.xml and two.xml, and two.xml used interfaces defined in
>>>>> one.xml. Is that an acceptable limitation?
>>>>>
>>>>> As a stretch I'd rather avoid for now, there could be an option to
>>>>> prefix all external interfaces with another prefix, but it would not be
>>>>> able to make a difference between things from wayland.xml and one.xml,
>>>>> for instance.
>>>>
>>>>
>>>> I think I'd like to avoid that too...
>>>>
>>>>> Where are we at with the name collision problem in general now?
>>>>
>>>>
>>>> Bit of a necropost, but was any kind of consensus in reach here? :)
>>>>
>>>> Emil's nominated this for inclusion in an upcoming release, and it would
>>>> certainly get us out of the xdg-shell-v5 vs xdg-shell-stable collisions
>>>> we face now (if we intend to have weston support xdg-shell-stable
>>>> without dropping v5 at some point).
>>>>
>>>> Does this have a strong use case outside of just dealing with the v5 vs
>>>> stable thing?  We could probably solve that with a sed script (in
>>>> wayland-protocols?)
>>>
>>>
>>> Hi,
>>>
>>> the original proposal was to prefix ABI symbols, and leave everything
>>> else as is. Maybe writing down again how exactly that is supposed to be
>>> used and what problems it solves would crystallize the idea. Perhaps in
>>> the form of wayland-scanner user instructions which the original patch
>>> seems to be lacking?
>>>
>>> Earlier I didn't like prefixing only some bits of the API, but on
>>> second thought, if that's all what's needed, maybe it isn't that bad
>>> from hand-written code readability point of view?
>>>
>>> The discussion showed that any other solution becomes hard and/or messy
>>> compared to that. This is not a library ABI/API change either, just a
>>> new operation mode in wayland-scanner.
>>>
>>> I join you with the question about use cases and how badly do we need
>>> this.
>>
>>
>> I don't see the point in keeping xdg-shell unstable v5 support in
>> weston, so from that point of view, *weston* I'd say don't need any
>> symbol prefix feature.
>>
>
> Ok, I guess at this point we should drop it from consideration for the next
> release.
>
> Pekka's synopsis is a really good status report for anyone interested in
> picking this up later without reading all the previous threads.
>
Something I mentioned over IRC, but forgot to add it here.
QT5 has been using unstable protocols for a while now. Combine that with:
 - interface symbols are exported by default
 - binary-only applications ship with their own version of QT (some
even have wayland-client.so and wayland-egl.so...)
 - some of those won't be updated to use the stable interface

This brings us to the original topic - symbol collusion. Jonas' idea
sounds reasonable to me.

-Emil
On Mon, 22 Jan 2018 14:46:15 +0000
Emil Velikov <emil.l.velikov@gmail.com> wrote:

> On 19 January 2018 at 15:49, Derek Foreman <derekf@osg.samsung.com> wrote:
> > On 2018-01-19 01:22 AM, Jonas Ådahl wrote:  
> >>
> >> On Thu, Jan 18, 2018 at 10:48:14AM +0200, Pekka Paalanen wrote:  
> >>>

> >>>
> >>> the original proposal was to prefix ABI symbols, and leave everything
> >>> else as is. Maybe writing down again how exactly that is supposed to be
> >>> used and what problems it solves would crystallize the idea. Perhaps in
> >>> the form of wayland-scanner user instructions which the original patch
> >>> seems to be lacking?
> >>>
> >>> Earlier I didn't like prefixing only some bits of the API, but on
> >>> second thought, if that's all what's needed, maybe it isn't that bad
> >>> from hand-written code readability point of view?
> >>>
> >>> The discussion showed that any other solution becomes hard and/or messy
> >>> compared to that. This is not a library ABI/API change either, just a
> >>> new operation mode in wayland-scanner.
> >>>
> >>> I join you with the question about use cases and how badly do we need
> >>> this.  
> >>
> >>
> >> I don't see the point in keeping xdg-shell unstable v5 support in
> >> weston, so from that point of view, *weston* I'd say don't need any
> >> symbol prefix feature.
> >>  
> >
> > Ok, I guess at this point we should drop it from consideration for the next
> > release.
> >
> > Pekka's synopsis is a really good status report for anyone interested in
> > picking this up later without reading all the previous threads.
> >  
> Something I mentioned over IRC, but forgot to add it here.
> QT5 has been using unstable protocols for a while now. Combine that with:
>  - interface symbols are exported by default
>  - binary-only applications ship with their own version of QT (some
> even have wayland-client.so and wayland-egl.so...)
>  - some of those won't be updated to use the stable interface
> 
> This brings us to the original topic - symbol collusion. Jonas' idea
> sounds reasonable to me.

Hi Emil,

all those issues sounds like they would be solved by not exporting
symbols and ensuring to use only local symbols for resolving, which are
both quite orthogonal. One cannot change a prefix every time when
building a different version of a protocol definition. Where not
exporting and local resolution would not be available, arguably neither
would prefixing.

Therefore I think we should concentrate on the no-export feature rather
than prefixing.

Let's try hard to drop xdg-shell v5 completely, just like originally
intended. I want to keep the distiction between unstable and stable.


Thanks,
pq
On 22 January 2018 at 15:09, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Mon, 22 Jan 2018 14:46:15 +0000
> Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
>> On 19 January 2018 at 15:49, Derek Foreman <derekf@osg.samsung.com> wrote:
>> > On 2018-01-19 01:22 AM, Jonas Ådahl wrote:
>> >>
>> >> On Thu, Jan 18, 2018 at 10:48:14AM +0200, Pekka Paalanen wrote:
>> >>>
>
>> >>>
>> >>> the original proposal was to prefix ABI symbols, and leave everything
>> >>> else as is. Maybe writing down again how exactly that is supposed to be
>> >>> used and what problems it solves would crystallize the idea. Perhaps in
>> >>> the form of wayland-scanner user instructions which the original patch
>> >>> seems to be lacking?
>> >>>
>> >>> Earlier I didn't like prefixing only some bits of the API, but on
>> >>> second thought, if that's all what's needed, maybe it isn't that bad
>> >>> from hand-written code readability point of view?
>> >>>
>> >>> The discussion showed that any other solution becomes hard and/or messy
>> >>> compared to that. This is not a library ABI/API change either, just a
>> >>> new operation mode in wayland-scanner.
>> >>>
>> >>> I join you with the question about use cases and how badly do we need
>> >>> this.
>> >>
>> >>
>> >> I don't see the point in keeping xdg-shell unstable v5 support in
>> >> weston, so from that point of view, *weston* I'd say don't need any
>> >> symbol prefix feature.
>> >>
>> >
>> > Ok, I guess at this point we should drop it from consideration for the next
>> > release.
>> >
>> > Pekka's synopsis is a really good status report for anyone interested in
>> > picking this up later without reading all the previous threads.
>> >
>> Something I mentioned over IRC, but forgot to add it here.
>> QT5 has been using unstable protocols for a while now. Combine that with:
>>  - interface symbols are exported by default
>>  - binary-only applications ship with their own version of QT (some
>> even have wayland-client.so and wayland-egl.so...)
>>  - some of those won't be updated to use the stable interface
>>
>> This brings us to the original topic - symbol collusion. Jonas' idea
>> sounds reasonable to me.
>
> Hi Emil,
>
> all those issues sounds like they would be solved by not exporting
> symbols and ensuring to use only local symbols for resolving, which are
> both quite orthogonal. One cannot change a prefix every time when
> building a different version of a protocol definition. Where not
> exporting and local resolution would not be available, arguably neither
> would prefixing.
>
> Therefore I think we should concentrate on the no-export feature rather
> than prefixing.
>
> Let's try hard to drop xdg-shell v5 completely, just like originally
> intended. I want to keep the distiction between unstable and stable.
>
Fair enough. Ignore I said anything ;-)

-Emil