thesofproject / sof

Sound Open Firmware
Other
546 stars 313 forks source link

How do we use tags on master branch? #3730

Closed dbaluta closed 3 years ago

dbaluta commented 3 years ago

This questions starts from sof-logger output:

ERROR FW ABI 0x3012001 DBG ABI 0x5002001 tag v1.5-rc1-996-g69073c6ae4db src hash 0xc4ec3834 (ldc hash 0xc4ec3834) This output looks to be correct because:

GIT_TAG = git describe --tags --abbrev=12 --match v* and $ git describe --tags --abbrev=12 --match v v1.5-rc1-996-g69073c6ae4db So, the question is how do we actually use tags? And what's their relation with our releases.

Cc: @xiulipan @marc-hb @paulstelian97 @TE-N-ShengjiuWang @lgirdwood @plbossart

paulstelian97 commented 3 years ago

I think a tag is made for every RC and release, and when doing git describe the most recent tag that points to a commit on the branch is selected. Not sure what happens if two tags point to the same commit though, but I mean I think we shouldn't get into that case anyway.

dbaluta commented 3 years ago

@paulstelian97 OK, it could be an explanation. But I have more questions:

lgirdwood commented 3 years ago

I would expect we would have the same rules as Linux here, but it looks like we are always adding the commit ID (even when commit is a tag) ?

paulstelian97 commented 3 years ago

I think src hash is used to identify a specific commit after the tag (tags do NOT move with new commits like branches do).

Tags point to fixed, specific commits. Tags for master are generally releases or RCs at least for this project.

In Git, tags and branches are almost the same, except branches evolve as new commits are added while tags stay attached to a specific commit. Checking out a tag always puts you in "detached HEAD" in fact.

dbaluta commented 3 years ago

@paulstelian97 @lgirdwood First thing we need to figure out why do we need the tag information in logs. I suspect that this is an easy way for the reader to identify release version.

lgirdwood commented 3 years ago

@ktrzcinx @abonislawski @slawblauciak any idea why the tag info is in logs ?

marc-hb commented 3 years ago

I see (at least) three distinct questions:

  1. Why would anyone not want git version information in logs that get attached to test runs and bug reports?
  2. Fine-tuning of git describe parameters. The current git describe parameters look OK to me, please be specific about how you would like to change them and why.
  3. The source hash is unrelated to the git version. The source hash makes sure the dictionary is compatible with the logger, see longer discussion in #3195
xiulipan commented 3 years ago

@ktrzcinx @abonislawski @slawblauciak any idea why the tag info is in logs ?

First answer why we have tag info, this is a part of our sof_ipc_fw_ready structure to dmesg log and work as HELLO WORLD TRACE log for trace system.(also see my email about 1.7 release yesterday)

dmesg
[    5.228533] kernel: sof-audio-pci 0000:00:1f.3: Firmware info: version 1:5:0-3c201
trace HELLO WORLD message
ERROR FW ABI 0x3012001 DBG ABI 0x5002001 tag v1.5-rc1-999-g3c20133263d5 src hash 0x2cfdece9 (ldc hash 0x2cfdece9)

@lgirdwood @dbaluta @paulstelian97 I see similar issue in our release process. We are making VERSION TAG from git tag. which means if a git tag is not on master branch, the git description will try to use the latest tag on the branch.

 git describe --tags --abbrev=12 --match v*
v1.5-rc1-999-g3c20133263d5

So take above output as example v1.5-rc1 - the latest tag with regex v* 999 - the master head now has 996 commits g3c20133263d5 - the head git commit is 3c20133263d5

git log v1.5-rc1..HEAD --oneline |wc
    999    8044   62576

Even thought we are in release v1.6.1, the TAG v1.6.1 is on the release branch and not on master branch.

To solve the issue we have 2 ways:

marc-hb commented 3 years ago

Even thought we are in release v1.6.1, the TAG v1.6.1 is on the release branch and not on master branch.

The master branch should not try to look like the v1.6.x branch because it's not, it has different code.

The reason the master branch looks like v1.5-rc1+something is because it is v1.5-rc1 plus something.

A typical process is to tag the first Release Candidate on the master branch, that's apparently what was done for v1.5-rc1. I don't know why it wasn't done for v1.6-rc1, see below.

*     2b54bacdb289 naveen.m@intel.com // topology: add KWD component into sof-tgl-sdw-max98373-rt5682
*     4c78e94a08eb yong.zhi@intel.com // topology: fix playback & capture pipelines to be timer driven
*     7bda1c831541 ranjani.sridharan@linux.intel.com // config: cht: remove tone comp
*     5abdf3778be8 ranjani.sridharan@linux.intel.com // volume: fix get_cmd() for SWITCH command
*     cad15f040501 karolx.trzcinski@linux.intel.com // rimage: Use only standard error codes
*     20fbbe77e474 yang.jie@linux.intel.com // topology: pipe-amp-ref-capture: correct the setting of ref channels
*     739554defaea yang.jie@linux.intel.com // topology: sof-tgl-nocodec-ci: remove KWD pipelines as not available
*     0a247928bb3f marc.herbert@intel.com // travis: upgrade distribution to Ubuntu 20.04
*     aa20048e92dc marc.herbert@intel.com // travis: fold long lines; fixes the last yamllint warnings
| *     7dd4c41d2e08 seppo.ingalsuo@linux.intel.com // (tag: v1.6-rc1) Audio: Continue volume ramp until all chann>
| *     478eb6874370 yong.zhi@intel.com // topology: fix playback & capture pipelines to be timer driven
| *     f3435914de2c ranjani.sridharan@linux.intel.com // config: cht: remove tone comp
| *     8af2c1da7c24 ranjani.sridharan@linux.intel.com // volume: fix get_cmd() for SWITCH command
| *     dc3015aa3e83 karolx.trzcinski@linux.intel.com // rimage: Use only standard error codes
| *     20fc71fad3ef yang.jie@linux.intel.com // topology: pipe-amp-ref-capture: correct the setting of ref channe>
| *     ab8a3564d750 yang.jie@linux.intel.com // topology: sof-tgl-nocodec-ci: remove KWD pipelines as not availab>
| *     2e259f45fe5a marc.herbert@intel.com // travis: upgrade distribution to Ubuntu 20.04
| *     61b521889ac2 marc.herbert@intel.com // travis: fold long lines; fixes the last yamllint warnings
|/  
*     6e98d9dde423 marc.herbert@intel.com // CODEOWNERS: really add marc-hb to scripts
*     c40749b9817f marc.herbert@intel.com // CODEOWNERS: fix path to dmic.
*     0e1176d9434a karolx.trzcinski@linux.intel.com // trace: Update trace level after ipc message
*     82c005c713d9 karolx.trzcinski@linux.intel.com // ipc: trace: Add trace filter message handling
*     7b5365f776c7 marcin.rajwa@linux.intel.com // dai: add warning when we skip .copy
*     dd5a56f37b91 karolx.trzcinski@linux.intel.com // rimage: Update to version with toml devices description
*     88b69cc2e5ca ranjani.sridharan@linux.intel.com // scripts: xtensa-build-all: Add support for building TGL an>
*     dfd2336bb2e7 marc.herbert@intel.com // cmake: empty MEU_OPENSSL param is now the same as not defined
marc-hb commented 3 years ago

I suggest the following "fix":

$ git tag v1.6-rc0 6e98d9dde423
$ git describe --match 'v*' --tags 3c20133263d5
v1.6-rc0-285-g3c20133263d5

Please thumbs up or down this github comment.

dbaluta commented 3 years ago

@marc-hb this means that we will need to manually update the current rc0 version each time we release a new version, right?

marc-hb commented 3 years ago

I'm not sure what you mean by "update the current rc0 version".

Ideally, every vN.M-rc1 release candidate (and only the first -rc1 release candidate) would be on the master branch. If that's not possible for some reason, then my suggestion above is to have a "dummy" vN.M-rc0 tag or vN.M-branchpoint tag on the master branch with no actual release candidate matching it.

dbaluta commented 3 years ago

@marc-hb OK, I understand your point now. Having vN.M-rc1 on master branch looks like an acceptable solution.

lgirdwood commented 3 years ago

@marc-hb I assuming you are proposing the same version schema that Linux uses here ? If so I approve.

marc-hb commented 3 years ago

Not sure what you mean by "version schema", I don't think I've been proposing any schema. I only proposed to tag the master branch near each release branch point which I think answered the main question.

lgirdwood commented 3 years ago

Not sure what you mean by "version schema", I don't think I've been proposing any schema. I only proposed to tag the master branch near each release branch point which I think answered the main question.

I mean the same schema used by Linux e.g. tag-[commit]-[dirty]

marc-hb commented 3 years ago

I mean the same schema used by Linux e.g. tag-[commit]-[dirty]

In other words: the git describe flags? I think/hope everyone is happy with the current ones.

xiulipan commented 3 years ago

@lgirdwood The schema is already decided by our git describe --tags --abbrev=12 --match v* cmd

The real problem here is the describe are using TAG on master branch, but out newer release TAG is not on master branch. What @marc-hb suggest here is to make a new TAG v1.6-rc0 on master branch so fix the issue.

AND for release 1.7 we should first make a TAG v1.7-rc1 on master branch and then checkout a new branch stable-v1.7 from this TAG.

lgirdwood commented 3 years ago

@lgirdwood The schema is already decided by our git describe --tags --abbrev=12 --match v* cmd

The real problem here is the describe are using TAG on master branch, but out newer release TAG is not on master branch. What @marc-hb suggest here is to make a new TAG v1.6-rc0 on master branch so fix the issue.

AND for release 1.7 we should first make a TAG v1.7-rc1 on master branch and then checkout a new branch stable-v1.7 from this TAG.

Ok, good explanation @xiulipan thanks - @dbaluta I think you should have the permissions to create these tags on master for v1.6. We should be tagging v1.7-rc1 in the next 2 weeks.

dbaluta commented 3 years ago

@lgirdwood ok, let me try.

dbaluta commented 3 years ago

$ git tag v1.6-rc0 6e98d9dde423

@marc-hb is there a particular reason for which you have mentioned this commit for rc0? I would have gone with v1.6-rc1^1

2b54bacdb topology: add KWD component into sof-tgl-sdw-max98373-rt5682

marc-hb commented 3 years ago

Yes, 6e98d9dde423 is the v1.6.x branch point. v1.6-rc1~1 is not on the master branch so tagging it would not make any difference. See my screenshot above.

dbaluta commented 3 years ago

@marc-hb I misused the commit v1.6-rc1~1. So, let me try to reprhase:

Here is how commits on master look:

$ git log --oneline 

d194fefbc rimage: update submodule to e67d68e8e1ea. More warnings fewer crashes
X: 1d6887f93 Audio: Continue volume ramp until all channels are complete [THIS is labeled (tag: v1.6-rc1) on v1.6 branch]
Y: 2b54bacdb topology: add KWD component into sof-tgl-sdw-max98373-rt5682 [THIS is what @dbaluta suggests as v1.6-rc0]
4c78e94a0 topology: fix playback & capture pipelines to be timer driven
7bda1c831 config: cht: remove tone comp
5abdf3778 volume: fix get_cmd() for SWITCH command
cad15f040 rimage: Use only standard error codes
20fbbe77e topology: pipe-amp-ref-capture: correct the setting of ref channels
739554def topology: sof-tgl-nocodec-ci: remove KWD pipelines as not available
0a247928b travis: upgrade distribution to Ubuntu 20.04
aa20048e9 travis: fold long lines; fixes the last yamllint warnings
Z: 6e98d9dde CODEOWNERS: really add marc-hb to scripts [THIS is what @marc-hb suggests for v1.6-rc0]

Please look at lines X, Y and Z. I suggest that commit at line Y to be labeled as v1.6-rc0 instead of commit at line Z.

Later edit: Wait you say 6e98d9d is the branchpoint. Let me check that. If so, then you are right.

marc-hb commented 3 years ago

To look at a graph of branches and see branchpoints you need either a graphical tool like gitk or git log --graph[*] git log without --graph cannot show anything useful. My screenshot above used --graph.

BTW an annotated tag would not hurt, I mean something like git tag -m 'This is just the branchpoint, not a real Release Candidate' v1.6-rc0 6e98d9d. Annotated tags will also please anyone running their own, different git describe commands without --tags for some reason.

[*] unless you look at the Linux kernel where it's a lost cause anyway.

mwasko commented 3 years ago

Generally the SOF release model is based on branches. This means that each new release will results in creating a new release branch like for example stable-1.6. Once the release branch gets created the process of stabilization and intensified validation begins which usually lead to creating multiple release candidates, each of them build from release branch and tagged ex. v.1.6-rc0, v.1.6-rc1.. In this working model the use of tags is quite useful even for description since you immediately see from the log with what release build you are working with.

I agree that the problem is with builds from master which basically should not use tag at all for FW description (imho). However we can also discuss the proposition to tag on master commits from which the community release branches were created 'v1.n' (but no 'rc' suffix since this point to specific release candidate build). The problem with tagging master with release tags is that someone may actually interpret the tag as actually stable point from which release took place so annotation proposed by Marc would be obligatory.

marc-hb commented 3 years ago

I find it worrying that the very first release candidate cannot be built from the branch point, that sounds bad. The mainline should be in state good enough to start validation at least sometimes! If that's really not the case then git tag [-a] v1.x-branchpoint abcdef is the second best option that solves the git describe problem reported in the description.

mwasko commented 3 years ago

I find it worrying that the very first release candidate cannot be built from the branch point, that sounds bad. The mainline should be in state good enough to start validation at least sometimes! If that's really not the case then git tag [-a] v1.x-branchpoint abcdef is the second best option that solves the git describe problem reported in the description.

The release usually involve new features which requires additional validation and may expose or introduce new bug's. Generally speaking the mainline should be stable enough to build the first release candidate but the new features may still require some polishing (that is added with next 'rc' builds). It would be quite confusing to have rc0 candidate on mainline followed by the next rc on the branch.

I am ok with the second option to use git tag [-a] v1.x-branchpoint abcdef on the mainline

dbaluta commented 3 years ago

what is v1.x-branchpoint exactly? So, how should this look like for the current master.

marc-hb commented 3 years ago

It would be quite confusing to have rc0 candidate on mainline followed by the next rc on the branch.

Confusing how, can you elaborate?

git users rarely ever look at git graphs anyway.

what is v1.x-branchpoint exactly?

The "branch point" is the last commit in common between the v1.x branch and master. For the v1.6 branch it's 6e98d9dde423, see the graph I posted above. In simple cases like this one the branch point can be found with the merge-base command but (as often with git...) the name "merge-base" isn't great here because: 1. we don't want to merge in this particular case 2. there can be multiple merge-bases in the general case whereas a branch is first created in only one place (that git does not record)

https://www.google.com/search?q=git+%22branch+point%22

cd sof
git merge-base --all master upstream/stable-v1.6
6e98d9dde423
marc-hb commented 3 years ago

Is git tag -a v1.6-branchpoint 6e98d9dde423 good enough for everyone?

xiulipan commented 3 years ago

@marc-hb I am OK with this workaround fix for release 1.6. @lgirdwood Do we have agreement here for release 1.7 to make first tag on master branch and then checkout to a new branch?

lgirdwood commented 3 years ago

@xiulipan @marc-hb yes this should all be happening now.

lgirdwood commented 3 years ago

@dbaluta I'm assuming we can close this now ?