apache / nuttx

Apache NuttX is a mature, real-time embedded operating system (RTOS)
https://nuttx.apache.org/
Apache License 2.0
2.65k stars 1.13k forks source link

zipme.sh is removing required release files #1363

Closed btashton closed 4 years ago

btashton commented 4 years ago

@liuguo09 noticed this issue with the 9.1.0 RC1 release tarball. We are removing important files like the rcS initd file from the sim:nsh build. When I ran a diff against the tarball and the git repo (excluding gitignore) I found other files including the asm for the z80 port that are being stripped. The output is this:

❯ diff -rq nuttx/ ../incubator-nuttx | grep -v gitignore
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f91_handlers.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f91_init.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f92_handlers.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f92_init.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f92_loader.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80f92_program.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_getsp.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_io.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_irqcommon.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_irqsave.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_progentry.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_reset.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_restorecontext.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_saveusercontext.asm
Only in ../incubator-nuttx/arch/z80/src/ez80: ez80_startup.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_head.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_restoreusercontext.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_rom.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_romvectors.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_saveusercontext.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_vectcommon.asm
Only in ../incubator-nuttx/arch/z80/src/z180: z180_vectors.asm
Only in ../incubator-nuttx/arch/z80/src/z80: z80_head.asm
Only in ../incubator-nuttx/arch/z80/src/z80: z80_restoreusercontext.asm
Only in ../incubator-nuttx/arch/z80/src/z80: z80_rom.asm
Only in ../incubator-nuttx/arch/z80/src/z80: z80_saveusercontext.asm
Only in ../incubator-nuttx: .asf.yaml
Only in ../incubator-nuttx/boards/arm/samd5e5/metro-m4/scripts: nvm.srec
Only in ../incubator-nuttx/boards/arm/samd5e5/same54-xplained-pro/scripts: nvm.srec
Only in ../incubator-nuttx/boards/sim/sim/sim/src/etc: init.d
Only in ../incubator-nuttx: .git
Only in ../incubator-nuttx: .github
Only in nuttx/: .version

Note that the zipme.sh calls tar with these options

+ tar --exclude=.github --exclude=.asf.yaml --exclude-vcs-ignores --exclude-vcs -czf apache-nuttx-9.1.0-incubating.tar.gz nuttx

The .asm files are striped because of the .asm file in the .gitignore file in the base directory triggered by --exclude-vcs-ignores

The missing init.d I suspect is caused because inside that folder is a rcS file. RCS (unclear if it is case sensitive) is removed by the --exclude-vcs flag (RCS is an old version control system) see https://www.gnu.org/software/tar/manual/html_node/exclude.html

Ouss4 commented 4 years ago

The missing init.d I suspect is caused because inside that folder is a rcS file.

Isn't because it's being confused with make dependency files? In any case, this is an issue that needs to be fixed. However, I don't understand why the "*.asm" files are ignored in the first place. Those are our files, they should be kept under version control.

Ouss4 commented 4 years ago

I think short term solution is to remove the exclude-vcs-ignores option. One should make sure that the repo is clean, zipme does a distclean. Also any file that was added and is not under version control must be manually ignored, ex: ./tools/zipme.sh -e "*.bin" 9.1.0

patacongo commented 4 years ago

However, I don't understand why the "*.asm" files are ignored in the first place. Those are our files, they should be kept under version control.

My recollection is that there was recently a change by Xiao Xiang that reorganized and combined, gitignore files. It removed many .gitignore files and combined them at a higher level. I suspect that this introduced the error for .asm files. I think that for some tools, .asm is the proper name for assembly files. I think that for others, .asm are generated files.

If my suspicion is true, then the correct fix is to revert parts of that PR, Maybe #1106 ? I can't look now, I am using my phone.

patacongo commented 4 years ago

I think short term solution is to remove the exclude-vcs-ignores option. One should make sure that the repo is clean, zipme does a distclean. Also any file that was added and is not under version control must be manually ignored, ex: ./tools/zipme.sh -e "*.bin" 9.1.0

But if my hunch is correct, this would leave the .gitignore bug in the release.

Ouss4 commented 4 years ago

Yes some tools generate .asm files. BTW, is it possible to use the .S extension for Z80 assembly files?

But if my hunch is correct, this would leave the .gitignore bug in the release.

The.gitignore files will be removed by --exclude-vcs, --exclude-vcs-ignores excludes the extensions it reads from .gitignore.

xiaoxiang781216 commented 4 years ago

However, I don't understand why the "*.asm" files are ignored in the first place. Those are our files, they should be kept under version control.

My recollection is that there was recently a change by Xiao Xiang that reorganized and combined, gitignore files. It removed many .gitignore files and combined them at a higher level. I suspect that this introduced the error for .asm files. I think that for some tools, .asm is the proper name for assembly files. I think that for others, .asm are generated files.

If my suspicion is true, then the correct fix is to revert parts of that PR, Maybe #1106 ? I can't look now, I am using my phone.

Yes, I move .asm to the top level of .gitigonre since .asm appear in many sub .gitigonre. The build system can catch the orphan files error, so we can simplify remove .asm from the top level .gitigonre and see which folder really need this and add it back one by one.

xiaoxiang781216 commented 4 years ago

Here is the patch: https://github.com/apache/incubator-nuttx/pull/1364 let's see the result.

patacongo commented 4 years ago

BTW, is it possible to use the .S extension for Z80 assembly files?

No, unfortunately not. I do use .S file now and then and autogenerate the .asm files using the host cpp. But, in general, no, files cannot be renamed.

Ouss4 commented 4 years ago

GIT is still tracking the Z80 .asm files since they were there before the gitignore change. Any new file with the .asm extension will be ignored though. Here is a quick way to check: git check-ignore -v arch/z80/src/ez80/ez80_reset.asm This returns nothing since the file is cached. However git check-ignore -v --no-index arch/z80/src/ez80/ez80_reset.asm will show: .gitignore:3:*.asm arch/z80/src/ez80/ez80_reset.asm. The ".gitignore:3:*.asm" part is were that file is ignored (i.e. toplevel .gitignore line 3)

TAR, in the other hand, doesn't know that and is excluding all the .asm files.

Note however that this is just part of this issue, and that #1364 doesn't fully resolve it. We still have the "init.d" file that I can't personally see how to include it (other than removing the --exlcude-vcs-ignores).

patacongo commented 4 years ago

GIT is still tracking the Z80 .asm files since they were there before the gitignore change. Any new file with the .asm extension will be ignored though.

Yes, but there is still a bug. If you modify one of the .asm files, your modifications will not be included in the commit. That is very bad.

xiaoxiang781216 commented 4 years ago

@patacongo @btashton @Ouss4 #1364 pass the build which mean we can remove *.asm from .gitignore without problem.

Ouss4 commented 4 years ago

Yes, but there is still a bug. If you modify one of the .asm files, your modifications will not be included in the commit. That is very bad.

It is a bug but if you modify one of the existing files, the change will be included. If a new file is added, the whole file won't be.

@patacongo @btashton @Ouss4 #1364 pass the build which mean we can remove *.asm from .gitignore without problem.

We can merge then, this solves the Z80 .asm part. But please don't close this issue, we still have init.d to worry about.

xiaoxiang781216 commented 4 years ago

We can merge then, this solves the Z80 .asm part. But please don't close this issue, we still have init.d to worry about.

Ok, I remove #1364 from the link.

patacongo commented 4 years ago

I believe that the issue is the the ZDS-II toolchain assembly files have the extension .asm but when you compile a .c file it generates a temporary .asm file that is then assembled. When compile a foobar.c file with the ZDS-II toolchain, it generates foobar.asm, foobar.rel, foobar.obj, and foobar.lst. The .asm file is the only issue here because it is also the name used with all ZDS-II assembly file.

So /.asm must appear in all ZDS-II build directories but cannot appear in a higher level directory. Just removing .asm from the top-level .gitignore file means that all of these .asm files (and there are hundreds) will all appear as untracked files. That makes those configurations difficult to use.

This is a consequence of people making unnecessary change code when they do not understand the full ramifications and do absolutely no testing.

patacongo commented 4 years ago

Is there any .gitignore syntax to undo a higher level .gitignore file? We really only need to NOT ignore .asm files in those few low level directories where there the .asm files exist. They could be ignored everywhere else (since temporary .asm files will be generated for every compiled .c file).

Another option might be to rename the temporary .asm files as .tmpasm or something like that. Actually, the correct solution was in place before the problem PR broke everything.

Ouss4 commented 4 years ago

Is there any .gitignore syntax to undo a higher level .gitignore file?

This should do:

diff --git a/arch/z80/src/.gitignore b/arch/z80/src/.gitignore
index b9a85219a9..5ba8c45774 100644
--- a/arch/z80/src/.gitignore
+++ b/arch/z80/src/.gitignore
@@ -7,3 +7,4 @@
 /*.mem
 /*.linkcmd
 /*.noi
+!*.asm
patacongo commented 4 years ago

1364 should not be merged. It is incorrect. See the comments above and in #1364

My recommendation is to revert #1106. A modified version can be re-applied after the release.

xiaoxiang781216 commented 4 years ago

I believe that the issue is the the ZDS-II toolchain assembly files have the extension .asm but when you compile a .c file it generates a temporary .asm file that is then assembled. When compile a foobar.c file with the ZDS-II toolchain, it generates foobar.asm, foobar.rel, foobar.obj, and foobar.lst. The .asm file is the only issue here because it is also the name used with all ZDS-II assembly file.

So /.asm must appear in all ZDS-II build directories but cannot appear in a higher level directory. Just removing .asm from the top-level .gitignore file means that all of these .asm files (and there are hundreds) will all appear as untracked files. That makes those configurations difficult to use.

Your description isn't self-consistent: 1.If git ignore .asm, git can't track the manual written assembler files. 2.If git don't ignore .asm, the generated assembler files mess up the directory.

This is a consequence of people making unnecessary change code when they do not understand the full ramifications and do absolutely no testing.

it isn't good to duplicate .gitignore in each folder(I never see other project to use .gitignore like this). @patacongo since only you are familar with z80 arch and actively change the code in arch/z80 and boards/z80, please take some time to improve the build system to cover z80. This is only way to ensure z80 quality.

patacongo commented 4 years ago

Your description isn't self-consistent: 1.If git ignore .asm, git can't track the manual written assembler files. 2.If git don't ignore .asm, the generated assembler files mess up the directory.

No, you are missing an important point. Before the change, /.asm was used which effects ONLY the directory containing the .asm file. It does not propagate to lower directories as does .asm.

So before /*.asm occurred in the .gitnore of all directories that contained temporary .asm files and worked perfectly.

patacongo commented 4 years ago

Is there any .gitignore syntax to undo a higher level .gitignore file? We really only need to NOT ignore .asm files in those few low level directories where there the .asm files exist. They could be ignored everywhere else (since temporary .asm files will be generated for every compiled .c file).

Yes!

An optional prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again. It is not possible to re-include a file if a parent directory of that file is excluded. Git doesn’t list excluded directories for performance reasons, so any patterns on contained files have no effect, no matter where they are defined. Put a backslash ("\") in front of the first "!" for patterns that begin with a literal "!", for example, "!important!.txt".

So a solution would be to:

  1. Keep the *.asm in the top-level .gitignore.
  2. Add a .gitignore with !*.asm in all directories containing tracked .asm files.
patacongo commented 4 years ago

I believe that PR #1367 will fix the issue with the .asm files. Can you please verify that? This replaces PR #1364 (which I closed).

Ouss4 commented 4 years ago

Do you need to ignore them in every sub-directory? Doesn't the z80/src suffice? I suggested this above:

diff --git a/arch/z80/src/.gitignore b/arch/z80/src/.gitignore
index b9a85219a9..5ba8c45774 100644
--- a/arch/z80/src/.gitignore
+++ b/arch/z80/src/.gitignore
@@ -7,3 +7,4 @@
 /*.mem
 /*.linkcmd
 /*.noi
+!*.asm
patacongo commented 4 years ago

Do you need to ignore them in every sub-directory? Doesn't the z80/src suffice?

No, that will not work. All of the generated, temporary .asm files will exist in arch/z80/src/. Those are some that must be ignored. The .gitignore files must go at a level LOWER than arch/z80/src/. No temporary .asm files will exist there, only tracked .asm files.

This used to work fine before because arch/z80/src/.gitignore contained /*.asm which ignored the .asm files ONLY in that directory and not the lower level directories. We need to replicate that same behavior.

Ouss4 commented 4 years ago

This used to work fine before because arch/z80/src/.gitignore contained /*.asm which ignored the .asm files ONLY in that directory and not the lower level directories. We need to replicate that same behavior.

We can chain patterns like the following:

diff --git a/arch/z80/src/.gitignore b/arch/z80/src/.gitignore
index b9a85219a9..829a4e917c 100644
--- a/arch/z80/src/.gitignore
+++ b/arch/z80/src/.gitignore
@@ -7,3 +7,5 @@
 /*.mem
 /*.linkcmd
 /*.noi
+!*.asm
+/*.asm

First we undo the global .asm ignore then we ignore .asm only in the current directory.

patacongo commented 4 years ago

We can chain patterns like the following:

Except we still need to ignore the .asm files in the (copied) board sub-directory. Since there are no symbolic links in this particular Windows configuration, the board directory is copied under arch/z80/src/board and it will contain .rel, obj., .asm, and .lst files that must also be ignored.

The .gitignore file in all of the old board directories also contained /*.asm

The solution proposed by PR #1367 is the simplest that I am aware of given the current state of things.

Ouss4 commented 4 years ago

I was trying to replicate the old behavior, but apparently there is more to it. I merged #1367. @xiaoxiang781216 This means that we don't need to remove *.asm from the top-level .gitignore.

patacongo commented 4 years ago

Except we still need to ignore the .asm files in the (copied) board sub-directory. Since there are no symbolic links in this particular Windows configuration, the board directory is copied under arch/z80/src/board and it will contain .rel, obj., .asm, and .lst files that must also be ignored.

The .gitignore file in all of the old board directories also contained /*.asm

In this case (as in lots of cases) I misspoke. The board directory copy is not part of the repository so the .gitignore file does not matter if the directory is copied.

I would not matter in other cases where the board sub-directory is a symbolic link either because the board directory proper is under the top-level .gitignore and would ignore the generated .asm files in the board directories.

Ouss4 commented 4 years ago

To get back to the original issue: I guess this is critical enough to cancel the current RC. The .asm issue is no more, for the rest I suggest we remove --exclude-ignore-vcs from the zipme.sh script until we come up with something better. We can then backport #1367 and the new PR to the release branch and cut a new RC. (BTW, @patacongo @adamfeuer are those SAMD5 .srec files supposed to be in the repo?)

patacongo commented 4 years ago

(BTW, @patacongo @adamfeuer are those SAMD5 *.srec files supposed to be in the repo?)

That is SAME5D5; Adam is working SAMA5D.

Yes, the nvm.srec is needed for the Metro-M4 board. That board comes with the FLASH locked. The srec file will unlock the FLASH. I doubt that is it needed with the same54-xplained-pro, but I don't know. I don't have that board.

Ouss4 commented 4 years ago

Okay, thanks for the info. I see that there is a C program that generates those files, can't we just keep that?

The missing init.d I suspect is caused because inside that folder is a rcS file.

Isn't because it's being confused with make dependency files?

I checked that it is indeed due to the .d extension. It's not only a TAR issue, even Git is ignoring that directory, except for the rcS file. This is a rather new file that was added by #793, I'm not sure how it made it through since the .d ignore is 4 years old. @xiaoxiang781216 Did you have to add any new script under sim/src/etc/init.d? That won't be picked up by Git.

xiaoxiang781216 commented 4 years ago

git can't pick up the new file under init.d, but this patch can fix it: https://github.com/apache/incubator-nuttx/pull/1373.