Hi all,
First
of all, apologies for having missed the previous bi-weekly report.
An unfortunate combination of me being on holidays 1 week after
BlinkOn with the fact that my laptop's motherboard died right on
Nov 15th made it hard for me to send that report, so I'm updating
now with the progress from the entire month, sorry about that.
As usual, we kept focused on converting types outside of Blink +
migrating usages of InterfaceProvider API to the new
BrowserInterfaceBroker, which naturally helped getting rid of the
few old mojo types remaining inside Blink, getting us to the point
where there's only one instance left! We'll remove that one now
and then will propose a CL to enable presubmit errors inside
Blink, so that no old types get now introduced. Note that these
numbers now take into account the false positives that the grep
query would find, which have been now subtracted from the numbers
throughout the whole spreadsheet, see more details at [1].
As for //content and other directories, steady progress in there as well, with old mojo types in now reduced to less than 25% and 60% in those categories compared to what numbers used to look like one month ago, bringing the current progress of the project to 91% if we compared with the date we started, as you can see in the "Project Completion (%)" table I've added to the bottom of this email, after the graph reports.
Find more detail in the base spreadsheet at:
https://docs.google.com/spreadsheets/d/1khYVrfnN74RSDqqXgCFlG9fI89IKWA9J32Ln_2ZcTaU
Thanks,
Mario
[1] https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/9vuGL48Lxik/6YscqLI5AwAJ
Hi again,
After sending this mail yesterday, we had an internal discussion
on what to do with //components/arc that we think might be worth
bringing it up here as well, since a recent comment by Jeremy
Roman in [1] made us realize not everyone might be aware of the
issue at hand...
[...]
As you can see from the numbers above, there are 37 instances of old mojo types to be migrated inside //components according to the grep query we're using, and more than half of those 37 instances (20, to be exact) are inside //components/arc:
3
components/arc/bluetooth/bluetooth_mojom_traits_unittest.cc
2 components/arc/keymaster/arc_keymaster_bridge.cc
1 components/arc/midis/arc_midis_bridge.cc
2 components/arc/session/arc_bridge_host_impl.cc
2 components/arc/session/arc_bridge_host_impl.h
1 components/arc/session/arc_session_impl.cc
5 components/arc/session/connection_holder.h
2 components/arc/session/mojo_channel.h
1 components/arc/timer/arc_timer_bridge.h
1 components/arc/wake_lock/arc_wake_lock_bridge.h
We did an initial attempt to migrate those bits last month as
part of crrev.com/c/1868870, but got a CR-1 because it seems that
Android's libchrome repository is not yet ready to handle changes
in the .mojom files from Chromium to the new types. The issue
seems to be that those .mojom files are copied into the libchrome
repo, which does not contain support for the new mojo types yet,
so that process would not work anymore like that, see [2] for more
details.
This is why we haven't prioritized converting types in
//components/arc for now, but now we're approaching the completion
of this migration project this question becomes relevant again:
what to do with //components/arc?
Note that, as Ken commented in [3], the old and new mojo types are binary compatibles over the wire, so I believe there should be a way to move forward with the new types in the Chromium repo even if libchrome needs to stick to the old ones for a bit longer, but that would require some extra discussion, so adding Hidehiko to Cc here.
Thoughts?
Thanks,
Mario
[1] https://chromium-review.googlesource.com/c/chromium/src/+/1940185/2#message-5cab01a211f6c201ef4e39a13c8a5c62e839f467
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1868870/6#message-5a35c25604873c017644aca5d739f39ca452fe4f
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1868870/6#message-77c2cadc1978b50d739b05cda7f64156b067fe51
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/25c57f3a-7ffc-d2e2-71c1-a1c6d091fc54%40igalia.com.
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely.Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?dave.
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAHgVhZViJgSCqUWwCTwAy4F3PWO9SKGCp-yi9KFH_a-dUHtUkQ%40mail.gmail.com.
Hi,
On 3/12/19 15:46, 'Ken Rockot' via chromium-mojo wrote:
On Tue, Dec 3, 2019 at 6:38 AM Dave Tapuska <dtap...@chromium.org> wrote:
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely.
Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?dave.
I think it's been done manually, but AFAIK typically the roll happens in the other direction -- source of truth in cros repo, rolled into chromium.
Do you mean "source of truth in libchrome (Android)
repo?" It's the first time I see CrOS mentioned in this topic,
that's where my question comes from :-)
Automating translation in that direction is challenging because the old notation for interface remotes is grammatically ambiguous.
I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.
I agree it's definitely easier to roll cross-repo mojoms in the
opposite direction (i.e. source of truth in Chromium repo) for the
reasons you mentioned, but I'm not sure what the actual next steps
to make that happen would be... I assume some devs who work on the
other repo should obviously get involved, but not sure who to ping
on that. Any idea?
It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.
Just to double-check I'm getting this right... what you mean here is that, we could actually go ahead and change the local copy of the .mojom files in Chromium as long as the original sources in the other repository don't change frequently, right?
We'd still need some agreement from the people working on that other repo on (1) that changes are not frequent and (2) that they would be happy to start off a point where .mojom file would not use the same -old- types in both repos and take it from there, but that would certainly unblock the conversions in Chromium repo.
WDYT?
Thanks again,
Mario
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANPWpyDTDnH5XteDi7E4vqPSidTic_JVxXjuNPx3EirZOg5pPg%40mail.gmail.com.
Some questions seem based on some misunderstanding of the current situation, so let me give it a try to explain.> We did an initial attempt to migrate those bits last month as part of crrev.com/c/1868870, but got a CR-1 because it seems that Android's libchrome repository is not yet ready to handle changes in the .mojom files from Chromium to the new types. The issue seems to be that those .mojom files are copied into the libchrome repo, which does not contain support for the new mojo types yet, so that process would not work anymore like that, see [2] for more details.Two git repos are related, libchrome and ARC, both are a part of Android..mojom files are copied from Chrome's component/arc to ARC, not to libchrome. It is built with libchrome.Current libchrome in Android is not new enough, so new syntax is not available yet.As for libchrome, there are two libchrome, one in Android (the one explained just above), and the other in Chrome OS.libchrome is the library extracted from Chrome repository. Now Android's one is upstream, and the it is downstreamed into Chrome OS's one.I.e., files are copied: Chrome -> Android's libchrome -> Chrome OS libchrome.There's a plan to swap the libchrome's upstream. Once it happens, the flow will be Chrome -> Chrome OS libchrome -> Android's libchrome. (probably, this is what Ken said).Next libchrome uprev will be blocked by the upstream swap. The upstream swap is now blocked by Chrome OS libchrome uprev, which is actively on-going.Once swap happens, Chrome OS libchrome will be uprev'ed more frequently. TBD for Android libchrome. We need a plan, based on Android is released once a year, IMHO.I think libchrome uprev including new syntax will happen eventually, but it will take time.> Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy?Copying mojom files are totally manual operation. In most cases, just "cp" command. No script, AFAIK.> Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?It introduces technical complexity to mojom update procedure for ARC.Also, it makes it difficult to take diff mechanically, thus it makes it difficult to find accidentally stale mojom files. In past, I took just diff, which wouldn't work.I'm not very sure if ARC would like to accept such a change. CC'ed several ARC core engs.
To me, the straightforward approach is technically, update libchrome in Android, and then use new syntax in ARC's mojom files.Thanks,- hidehikoOn Mon, Dec 9, 2019 at 9:33 PM Mario Sanchez Prada <ma...@igalia.com> wrote:Hi again,
On 4/12/19 10:49, Mario Sanchez Prada wrote:
> [...]
>> I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.
>
> I agree it's definitely easier to roll cross-repo mojoms in the opposite direction (i.e. source of truth in Chromium repo) for the reasons you mentioned, but I'm not sure what the actual next steps to make that happen would be... I assume some devs who work on the other repo should obviously get involved, but not sure who to ping on that. Any idea?
Pinging here after a few days.
>> It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.
>
> Just to double-check I'm getting this right... what you mean here is that, we could actually go ahead and change the local /copy/ of the .mojom files in Chromium *as long as* the original sources in the other repository don't change frequently, right?
>
> We'd still need some agreement from the people working on that other repo on (1) that changes are not frequent and (2) that they would be happy to start off a point where .mojom file would not use the same -old- types in both repos and take it from there, but that would certainly unblock the conversions in Chromium repo.
>
> WDYT?
We'd still need to clarify this part as well, in order to determine how to move forward.
Thanks again,
Mario
--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANPWpyDTDnH5XteDi7E4vqPSidTic_JVxXjuNPx3EirZOg5pPg%40mail.gmail.com.
On Tue, Dec 3, 2019 at 6:38 AM Dave Tapuska <dtap...@chromium.org> wrote:Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely.Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?dave.I think it's been done manually, but AFAIK typically the roll happens in the other direction -- source of truth in cros repo, rolled into chromium.Automating translation in that direction is challenging because the old notation for interface remotes is grammatically ambiguous.I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CA%2BapAgFYMJXvJr5-t%3Dg7%3DvRdBgaCuYdx_S%3DJvZRVkBMswJpe6Q%40mail.gmail.com.
> Can we have an estimate when these libchrome changes will be done? Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining. If these libraries take another year to update I don't believe that is an acceptable solution on our end.Current libchrome uprev focus is Chrome OS, and TBD for Android's libchrome (At least there's no plan in Q1). CC'ed libchrome team.I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.
Hi,
On Fri, Dec 20, 2019, 1:28 AM Hidehiko Abe <hide...@google.com> wrote:
> Can we have an estimate when these libchrome changes will be done? Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining. If these libraries take another year to update I don't believe that is an acceptable solution on our end.
Current libchrome uprev focus is Chrome OS, and TBD for Android's libchrome (At least there's no plan in Q1). CC'ed libchrome team.
I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.
If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.
Sounds reasonable to me. The script would be trivial to write and maintain.
Looks like we have a plan then, thank you all for your feedback and ideas!
I'm going OoO today until January 7th (back on that day), so I'll take a look into writing that script then if nobody beats me to it. I'll probably start with a bash script and see how it goes, but if it gets too ugly I'll likely switch to python, which should be ok too from a build-time perspective I think.
Anyway, time to go now... happy holidays and happy new year in advance!
Thanks,
Mario
PS: I'm not sure I'll be able to send the status report on the 1st of January due to me being OoO, so I'll just share here that //content has been fully migrated now too, joining Blink in the 100% achievement! See https://docs.google.com/spreadsheets/d/1khYVrfnN74RSDqqXgCFlG9fI89IKWA9J32Ln_2ZcTaU/edit#gid=346422195
Hi all,
On 20/12/19 16:17, 'Ken Rockot' via chromium-mojo wrote:
On Fri, Dec 20, 2019, 1:28 AM Hidehiko Abe <hide...@google.com> wrote:
[...]
I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.
If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.
Sounds reasonable to me. The script would be trivial to write and maintain.
Looks like we have a plan then, thank you all for your feedback and ideas!
I'm going OoO today until January 7th (back on that day), so I'll take a look into writing that script then if nobody beats me to it. I'll probably start with a bash script and see how it goes, but if it gets too ugly I'll likely switch to python, which should be ok too from a build-time perspective I think
I've taken a look to having such a script this morning and came up with a pretty simple one that takes the target directory as its only parameter and converts all the .mojom files inside of it from the new to the old types, using sed.
It's attached as mojo_types_converted.sh in this mail,
but the gist of it is this:
@hidehiko I considered your comment on avoiding sed here,
but I honestly think it's good enough since the bits we'd be
translating are never split across line breaks AFAICS, and using sed
seemed simple yet powerful enough to me in this particular case,
where we're only translating the 4 cases mentioned above. WDYT?
Also, for testing purposes, I ran it over the src/third_party/blink/public/mojom directory and then attached the output of running `git diff` afterwards in mojo_types_converter.diff. See below an excerpt of it:
[...]
It's attached as mojo_types_converted.sh in this mail, but the gist of it is this:
find "${SRCDIR}" -type f -name "*.mojom" | while read file; dosed --in-place --regexp-extended \-e 's/pending_remote<(.*)>/\1/g' \-e 's/pending_receiver<(.*)>/\1\&/g' \-e 's/pending_associated_remote<(.*)>/associated \1/g' \-e 's/pending_associated_receiver<(.*)>/associated \1\&/g' \"${file}"done
Actually, this is not correct, as sed's greedy nature means that
it could end up doing the wrong conversion sometimes, as it was
the case for native_file_system_file_writer.mojom:
Thus, we should avoid that, which we can do with a minor change
to the regular expression:
See attached the updated script and diff after running this new
script over Blink's mojom files.
Apologies for the extra noise. Everything else from my previous
email still applies, though
Thanks
Mario
Hi,I think we have miscommunication.Actually, running "sed" to conver mojom files looks fragile. E.g., even if just adding a linebreak after "<" due to the length of
identifiers, the script wouldn't work,but it is still valid in mojom_bindings_generator, IIUC.What I asked was; Could you give it a try to parse the mojom file by the actual parser, and generate a code which is compatible, so that it is guaranteed that the script works for any mojom files supported by mojom_bindings_generator?
Hi,
[...]
On Fri, Jan 10, 2020, 5:09 AM Hidehiko Abe <hide...@google.com> wrote:
Hi,
I think we have miscommunication.Actually, running "sed" to conver mojom files looks fragile. E.g., even if just adding a linebreak after "<" due to the length of
I agree that in theory it seems fragile, but in practice the use of such a script is a temporary concern and we have hundreds of mojoms over years of development without a line break in these type names. So very unlikely to be a real issue.
This was my thinking exactly: I realize that a sed line might not
be perfect and future-proof if we eventually start getting
affected by line breaks, but considering that this is a temporary
measure to work around this situation while those libchrome repos
don't support the new types, I thought it could be a good
trade-off.
identifiers, the script wouldn't work,but it is still valid in mojom_bindings_generator, IIUC.What I asked was; Could you give it a try to parse the mojom file by the actual parser, and generate a code which is compatible, so that it is guaranteed that the script works for any mojom files supported by mojom_bindings_generator?
Doing this would require changes to the parser, which is part of the repo we're trying to uprev. We would have to develop this change within the libchrome repo and update the dependency in each repo that uses it, wouldn't we?
I worry that this would be significantly more complex than having a simple copyable script and separate build step that can be stuffed into individual builds as needed.
FWIW, I also share these concerns. Personally, I was hoping we could move on with the simpler approach proposed, since this is the last roadblock preventing us from removing the old types from the chromium repository, but it's not clear to me at the moment whether that would be accepted. So, I think it would be good to try to clarify what the situation is, first of all.
hidehiko@ +others: WDYT? Would you be open in the end to this simpler approach of using a sed script? Or would you rather push for a more complex alternative?
Thanks,
Mario
Thanks,
Mario
Hi all,
First of all apologies for the late reply, see my comments below:
On Thu, Jan 23, 2020 at 4:24 AM Hidehiko Abe <hide...@google.com> wrote:
[...]
Had a chat with yusukes@ offline, and it looks like to be ok for him.To be clear, my alternate proposal is;1) Check in the downconver script to chromium repository.2) Add unittests which apply the downcoverter script to each mojom files exported to ARC, then parse the results by the older parser to make sure it wouldn't generate wrong code.In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests.
Mario, Ken, WDYT?
(CC+=haraken@, thank you for helping)
I miss the step in which the downconverter script will be
integrated with the libchrome repos, but I assume that will be
done independently in there once this script gets checked in for
chromium. Is that correct?
I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.
IIUC, what you're suggesting is to add a new command line argument to mojom_bindings_generator.py (e.g. --force-old-mojo-syntax?) that would only parse and generate bindings for .mojom files using the old syntax, and then add a new test case to mojom_bindings_generator_unittest.py to validate that the "downconverted" .mojom files can be successfully parsed and generated code for, right?
One thing I'm not 100% sure about, though, is what you mean by "forked old parser"... isn't the only parser (currently supporting new and old syntax) the one in mojo/public/tools/bindings/pylib/mojom/parse/parser.py, referenced from mojom_bindings_generator.py?
Thanks again,
Mario
Hi all,
First of all apologies for the late reply, see my comments below:
On 23/1/20 18:20, 'Ken Rockot' via chromium-mojo wrote:
On Thu, Jan 23, 2020 at 4:24 AM Hidehiko Abe <hide...@google.com> wrote:
[...]
Had a chat with yusukes@ offline, and it looks like to be ok for him.To be clear, my alternate proposal is;1) Check in the downconver script to chromium repository.2) Add unittests which apply the downcoverter script to each mojom files exported to ARC, then parse the results by the older parser to make sure it wouldn't generate wrong code.In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests.
Mario, Ken, WDYT?
(CC+=haraken@, thank you for helping)
I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?
Hi all,
Sorry again for the slowness replying here again, got my head wrapped around a different task and didn't realize of this reply...
On 29/1/20 17:44, 'Hidehiko Abe' via chromium-mojo wrote:
> [...]
> I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?
>
> It is not automatic procedure. The script is necessary to be added to libchrome in several ARC branches in Android repository.
Ok, but just to handle expectations: can we move ahead proposing the necessary changes in chromium (i.e. add the script + changes to mojom_binding_generator.py and mojom_binding_generator_unittest.py files) assuming that someone else working on libchrome will add the script to those several branches?
>> I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.
>
> IIUC, what you're suggesting is to add a new command line argument to mojom_bindings_generator.py (e.g. --force-old-mojo-syntax?) that would only parse and generate bindings for .mojom files using the old syntax, and then add a new test case to mojom_bindings_generator_unittest.py to validate that the "downconverted" .mojom files can be successfully parsed and generated code for, right?
This hasn't been confirmed or otherwise yet, please speak up if it's not correct. I was planning to start working on this soon (this week or next time, once I clear some roadblocks on a different task I'm currently working on). Thanks!
> One thing I'm not 100% sure about, though, is what you mean by "forked old parser"... isn't the only parser (currently supporting new and old syntax) the one in mojo/public/tools/bindings/pylib/mojom/parse/parser.py, referenced from mojom_bindings_generator.py?
Still a valid question.
Hi all,
On Wed, Feb 5, 2020 at 6:26 PM Mario Sanchez Prada <ma...@igalia.com> wrote:
[...]
Ok, but just to handle expectations: can we move ahead proposing the necessary changes in chromium (i.e. add the script + changes to mojom_binding_generator.py and mojom_binding_generator_unittest.py files) assuming that someone else working on libchrome will add the script to those several branches?
I was thinking you would maintain the build...?At least, I don't think I myself have enough capacity to maintain this, unfortunately.
As we (Kentaro, Hidehiko & me) discussed offline, I probably won't able to maintain the libchrome build myself, so someone else (from the libchrome or arc-core team maybe) would have to take care of that part.
From my side, though, I can as promised provide a version of the script that we can check in the chromium repository along with some kind of unittests, so I've spent some time today and yesterday working on that, and put the result of my work so far in https://chromium-review.googlesource.com/c/chromium/src/+/2041759
As you can see in there, I ended up writing the script in Python instead of bash since I think it's perhaps a better idea (more clear, more maintainable, better integration with unittests...), and also took that chance to write it in a way so that it works with odd spaces and linebreaks, which should address one of the concerns from @hidehiko.
I also added a new unittest to check all the cases I could think of (including multiline declarations, linebreaks and response callbacks), made it so that it would use a modified version of the current parser & lexer that won't accept the new Mojo types... and everything passes for me locally, so probably moving it to review later today.
@rockot: I've tried to implement the --force-old-mojo-syntax idea you suggested to avoid checking-in the a separate parser & lexer for this, but I concluded it was not as easy because the way parser.py and lexer.py are handled by the yacc.py module does not make it easy (i.e. I couldn't find a way) to reuse those files. So, I ended up removing the bits I didn't want to parse out of them and saving them as oldparser.py and oldlexer.py, which I believe is what Hidehiko suggested would have to happen in his previous mail[*], so hoping it's not bad).
I hope this helps untangle this situation. Once this script lands in chromium and we get an agreement on that someone will take care of using it for libchrome, we should be able to resume work on http://crrev.com/c/1868870 and migrate //components/arc to the new mojo types once and for all.
PTAL and let me know what you think.
Thanks!
Mario
[*] From Hidehiko's previous mail: "In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests."
--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/7336bd4a-660c-d873-9dd8-a2179fc29d53%40igalia.com.
Hi Ken,
Thanks for putting together that CL and exploring these options in such detail. It's really helpful.
Can you clarify what the --force-old-mojo-syntax switch was going to do?
As I mentioned earlier on this thread, I understood that the plan
was to modify the parser (not the generator) so that it
would only accept the old mojo types if --force-old-mojo-syntax
was passed, producing errors such as "Unexpected '<':"
otherwise if new types like PendingRemote<T> were found in
the source .mojom file.
This would allow me to test that whatever my downconverter script
produced was still parsable by a parser accepting the old types
only, which would be something I'd use on the unittests. This
would be inline, I believe, with what Hidehiko proposed earlier on
this thread (i.e. checking-in the script + unittests).
Unfortunately, looking at how the yacc parser and the lexer are
written, passing a parameter to modify their behaviour didn't seem
obvious at all, and ended up checking in old versions of both and
not implementing that parameter at all (which again seemed to
match what Hidehiko anticipated too).
In my mind at least, we would only have needed to touch mojom_cpp_generator.py and superficially have the flag emit InterfacePtr and InterfaceRequest any time we would have normally emitted PendingRemote and PendingReceiver. In theory we should be able to cherry-pick a newer generator into libchrome since I don't *think* we've made any backwards-incompatible changes to it in the past few years.
Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2043885
...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.
I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.
WDYT?
Thanks,
Mario
WDYT?
Thanks,
Mario
Hi Ken,
[...]
Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:https://chromium-review.googlesource.com/c/chromium/src/+/2043885
...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.
I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.
Sorry, my fault for not being more clear in the last message. Your understanding of things was 100% accurate.
No problem, thanks for clarifying.
The relative complexity of the changes needed to support down-conversion -- compared to the original proposal of a one-line sed filter -- started to make me wonder if generator changes could be the simpler approach after all.
Well, the second CL I've just uploaded seems simple enough to be honest, and perhaps easier to integrate in external repos like libchrome (i.e. just pass --force_old_mojo_types), but I'm finding it a bit harder to add unit tests for it than in the other CL that included the downgraded script.
Either way, I'm happy to move forward with whichever option that looks better to you (Ken) and Hidehiko (others' comments welcome too, of course).
PTAL and let me know what you prefer.
Thanks,
Mario
Thanks,
Mario
On Mon, Feb 10, 2020 at 7:54 AM Mario Sanchez Prada <ma...@igalia.com> wrote:Hi Ken,
On 10/2/20 14:23, 'Ken Rockot' via chromium-mojo wrote:
[...]
Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:https://chromium-review.googlesource.com/c/chromium/src/+/2043885
...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.
I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.
Sorry, my fault for not being more clear in the last message. Your understanding of things was 100% accurate.No problem, thanks for clarifying.
The relative complexity of the changes needed to support down-conversion -- compared to the original proposal of a one-line sed filter -- started to make me wonder if generator changes could be the simpler approach after all.Well, the second CL I've just uploaded seems simple enough to be honest, and perhaps easier to integrate in external repos like libchrome (i.e. just pass --force_old_mojo_types), but I'm finding it a bit harder to add unit tests for it than in the other CL that included the downgraded script.
Either way, I'm happy to move forward with whichever option that looks better to you (Ken) and Hidehiko (others' comments welcome too, of course).
PTAL and let me know what you prefer.
I agree that the second CL seems simpler. I think we could get by with a simple compile test, i.e. add GN support to optionally set the force_old_mojo_types flag, define a mojom interface method that takes a pending_receiver<> and pending_remote<> type, and statically assert in some existing unit test file (maybe new_endpoint_types_unittest.cc) that the signature of the generated interface uses mojo::InterfaceRequest<> and mojo::InterfacePtr<>.I would like to hear back from Hidehiko though regarding his thoughts on the feasibility of partially uprevving only the bindings generator.
On Mon, Feb 10, 2020 at 10:18 AM Ken Rockot <roc...@google.com> wrote:
On Mon, Feb 10, 2020 at 7:54 AM Mario Sanchez Prada <ma...@igalia.com> wrote:
[...]
PTAL and let me know what you prefer.
I agree that the second CL seems simpler. I think we could get by with a simple compile test, i.e. add GN support to optionally set the force_old_mojo_types flag, define a mojom interface method that takes a pending_receiver<> and pending_remote<> type, and statically assert in some existing unit test file (maybe new_endpoint_types_unittest.cc) that the signature of the generated interface uses mojo::InterfaceRequest<> and mojo::InterfacePtr<>.
I would like to hear back from Hidehiko though regarding his thoughts on the feasibility of partially uprevving only the bindings generator.
(CC+=satorux@, my manager for visibility).
From libchrome perspective, downcovert script looks simpler and preferred from ARC maintenance point of view.Technically, indeed it probably could be possible to take newer mojom_bindings_generator exposed as a standard repository to Android and Chrome OS. However, there is non-trivial amount of work needed.
With some discussion, libchrome team (fqj@, hscham@, hidehiko@) can take a look Android build side.I will send PSA to arc-eng@, who're actually affected by this change.
Wonderful, thanks! I'll move the CL with the downconverter script to the review process then, and will abandon the other one for now. Once that lands and we get the green light from the libchrome team, we could resume the conversion of the types in //components/arc, if that's ok.
Thanks again for your help & feedback with this,
Mario
BTW, are there any documents describing the diff from old syntax to new syntax?
Yes, the "Mojo Bindings Conversion Cheatsheet" describes the changes that have been done in the context of this migration, both to C++ code and .mojom files:
https://docs.google.com/document/d/1Jwfbzbe8ozaoilhqj5mAPYbYGpgZCen_XAAAdwmyP1E/edit#
Note that for the purpose of what's been discussed in this thread, we are only concerned with the changes in the .mojom files, that is, the following kind changes:
Foo& request <=> pending_receiver<Foo> request
Foo client <=> pending_remote<Foo>
client
associated Foo& request <=>
pending_associated_receiver<Foo> request
associated Foo client <=>
pending_associated_remote<Foo> client
...as those are the ones that would be imported by libchrome through ARC, and which need downgrading.
Also, do you have summary of the changes?
You can find all the CLs done in the context of the conversions done inside and outside of Blink so far in https://crbug.com/955171 and https://crbug.com/978694, respectively.
If you're asking about the changes that would be done in the context of //components/arc, there's a WIP CL in crrev.com/c/1868870 that I think summarizes well the changes that would follow in those ARC-related .mojom files once we get the green light.
Hope this helps!
Thanks,
Mario
Hi all,
On 13/2/20 8:49, Hidehiko Abe wrote:
[...]
(CC+=satorux@, my manager for visibility).
From libchrome perspective, downcovert script looks simpler and preferred from ARC maintenance point of view.Technically, indeed it probably could be possible to take newer mojom_bindings_generator exposed as a standard repository to Android and Chrome OS. However, there is non-trivial amount of work needed.
With some discussion, libchrome team (fqj@, hscham@, hidehiko@) can take a look Android build side.I will send PSA to arc-eng@, who're actually affected by this change.
Wonderful, thanks! I'll move the CL with the downconverter script to the review process then, and will abandon the other one for now.
Once that lands and we get the green light from the libchrome team, we could resume the conversion of the types in //components/arc, if that's ok.
@hidehiko Now waiting for this "green light" from the libchrome team to know when it would be ok to resume work on https://crrev.com/c/1868870. Also, I think we can end this long thread here now and continue further discussion in the bug at https://crbug.com/1035484.
Thanks again to everyone involved!
Mario
Hi Hidehiko,
[...]
Sorry for taking time. I discussed it with the libchrome team.As for eng resource allocation from the libchrome team, let me defer to chavey@, who's the manager of the team.
No problem at all, and thanks for helping with this. I'll wait for you to get back to me then before doing anything.
Thanks!
Mario