search

google / cro3

Abstraction Layer of ChromiumOS development
https://google.github.io/cro3/
BSD 3-Clause "New" or "Revised" License
36 stars 12 forks source link

Make sure lium build and deploy can be ran without cros_workon automatically (add `--unmodified` option to them) #49

Open HidenoriKobayashi opened 1 year ago

HidenoriKobayashi commented 1 year ago

While reviewing the PR on add --ab_update #46, I noticed cros-workon start is called for the packages to be deployed. Is this necessary? I feel if we do things like cros-workon behind the scene, that might be a source of confusion.

mhiramat commented 1 year ago

I thought cros-workon start <package> if for letting deploy command to use -9999 version of the package.

hikalium commented 1 year ago

In practice, more developer time is wasted by forgetting cros-workon start than doing cros-workon start unintentionally, since most of the cases we do deploying a package is to push a locally-modified package.

We can avoid the confusion by printing all the commands being executed in chroot in each step I think. It is also useful for educating developers how to use the commands in the chroot and make lium as a library of working examples of commands to build & modify & test the ChromiumOS.

What do you think...?

(Maybe we should document it in our design doc and also README.md or somewhere... or maybe ARCHITECTURE.md or something?)

HidenoriKobayashi commented 1 year ago

Hmm, okay. I didn't know that many people forget to cros-workon.

But it still leaves me a question: shouldn't that be detected when they build the package?

This is how I want the tool to behave if I forgot to cros-workon:

  1. edit source
  2. build --> lium warns me about not having cros-workon'ed the target package and let me do that interactively
  3. deploy

I sometimes find myself wanting to deploy the original package? I could put back the branch state, but I feel that would be more of a hassle. So for that, I want an option to skip cros-workon at deploy. But, for cases that I forget to cros-workon, I think it is better to be warned at build time.

(Well, I guess this discussion is also somewhat affected by relation to the original tool (cros) redesign and how lium want to position itself)

hikalium commented 1 year ago

First, as a principle, lium should fail fast (please see our internal doc... I should move that contents to the public DESIGN.md btw...). Therefore, asking the user to do cros-workon interactively on lium build is not an ideal option.

Also, regardless of what the cros tool is thinking, lium aims to provide a straight-forward way to "develop" chromiumos. And, in most of the cases, development is a continuous loop of change, build, deploy and test. cros-workon is just an artificial restriction made by the cros sdk, so lium should make it transparent as much as possible. That's why the current lium implementation changes the cros-workon state automatically.

How about adding "lium deploy --unmodified ..." option to ensure deploying the unmodified (crosworkon-stop-ped) packages?

HidenoriKobayashi commented 1 year ago

Thanks on the clarification of the principle! (BTW with regard to that, there was a time I wised one of the lium commands to check the consistency of arguments before partly starting the work. However, I forgot what it was. Sorry, I will get back to it at another time)

Okay, that should probably work for me, if lium build can also build the non cros-workon version of the package.

hikalium commented 1 year ago

Thank you too for asking for the clarification! Updated the issue title to make it clear what is expected. Let's work on it when someone has a free time ;)

HidenoriKobayashi commented 1 year ago

Hmm, this was little bit more complicated than I thought.

Adding --unmodified to build works nicely. If I set --unmodified, lium does cros-workon stop and build the package. Otherwise, lium does cros-workon start and build the package. So good, so far.

Adding --unmodified to deploy however turns out to be more difficult. As far as I tried, update_kernel.sh does not obey the workon state of the package. It seems to deploy the one that was built the last time. And it was the same for cros deploy. it spews a warning message if I try to deploy a package without workon. However, to my surprise, it still picks up the package with the suffix of 9999 if the last package was built during workon. The other way around also deploys the non-intended package. So, this is confusing. In order to match the user's expectation, we would actually need to build the package that matches the given option if the last-built package was not the one we want.

(BTW, this makes me wonder if the cros-workon start that we currently have in deploy is really doing anything.)

I guess I can live without the --unmodified options for now, until everybody (including the official tool) agrees on getting rid of the workon notion from deploy stage if ever.