From 6eec4b04a2d12e6efe7744ff1221597b15c9b2ed Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:21 +0100 Subject: [PATCH 01/44] sandbox: eth-raw: do not close the console input When the sandbox eth-raw device host_lo is removed this leads to closing the console input. Do not call close(0). Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/sandbox/cpu/eth-raw-os.c | 8 ++++---- arch/sandbox/cpu/os.c | 5 ++++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/arch/sandbox/cpu/eth-raw-os.c b/arch/sandbox/cpu/eth-raw-os.c index da01d1addf..6a8d809756 100644 --- a/arch/sandbox/cpu/eth-raw-os.c +++ b/arch/sandbox/cpu/eth-raw-os.c @@ -53,7 +53,7 @@ int sandbox_eth_raw_os_is_local(const char *ifname) } ret = !!(ifr.ifr_flags & IFF_LOOPBACK); out: - close(fd); + os_close(fd); return ret; } @@ -220,7 +220,7 @@ int sandbox_eth_raw_os_send(void *packet, int length, struct sockaddr_in addr; if (priv->local_bind_sd != -1) - close(priv->local_bind_sd); + os_close(priv->local_bind_sd); /* A normal UDP socket is required to bind */ priv->local_bind_sd = socket(AF_INET, SOCK_DGRAM, 0); @@ -284,11 +284,11 @@ void sandbox_eth_raw_os_stop(struct eth_sandbox_raw_priv *priv) { free(priv->device); priv->device = NULL; - close(priv->sd); + os_close(priv->sd); priv->sd = -1; if (priv->local) { if (priv->local_bind_sd != -1) - close(priv->local_bind_sd); + os_close(priv->local_bind_sd); priv->local_bind_sd = -1; priv->local_bind_udp_port = 0; } diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index e7ec892bdf..c461fb0db0 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -86,7 +86,10 @@ int os_open(const char *pathname, int os_flags) int os_close(int fd) { - return close(fd); + /* Do not close the console input */ + if (fd) + return close(fd); + return -1; } int os_unlink(const char *pathname) From b1ad4157638ed4f22d8ae59c434f694730163bca Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:22 +0100 Subject: [PATCH 02/44] sandbox: enable poweroff command The command to shut down a device is 'poweroff'. It is a deficit of the sandbox that it does not support resetting yet but shuts down upong seeing the 'reset' command. Once the sandbox properly supports reset we need the 'poweroff' command to leave the sandbox. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/Kconfig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/Kconfig b/arch/Kconfig index 3b9fcce980..18de8d807a 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -92,6 +92,7 @@ config SANDBOX bool "Sandbox" select BOARD_LATE_INIT select BZIP2 + select CMD_POWEROFF select DM select DM_GPIO select DM_I2C @@ -107,7 +108,7 @@ config SANDBOX select PCI_ENDPOINT select SPI select SUPPORT_OF_CONTROL - select SYSRESET_CMD_POWEROFF if CMD_POWEROFF + select SYSRESET_CMD_POWEROFF imply BITREVERSE select BLOBLIST imply CMD_DM From 35d949222f5fc629cf89063a39998fbc39c2c4ca Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:23 +0100 Subject: [PATCH 03/44] test/py: test poweroff It is the 'poweroff' and not the 'reset' command that should shut down the sandbox. Adjust the unit test accordingly Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- test/py/tests/test_sandbox_exit.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/py/tests/test_sandbox_exit.py b/test/py/tests/test_sandbox_exit.py index a301f4b559..2d242ae0f6 100644 --- a/test/py/tests/test_sandbox_exit.py +++ b/test/py/tests/test_sandbox_exit.py @@ -6,11 +6,11 @@ import pytest import signal @pytest.mark.boardspec('sandbox') -@pytest.mark.buildconfigspec('sysreset') -def test_reset(u_boot_console): - """Test that the "reset" command exits sandbox process.""" +@pytest.mark.buildconfigspec('sysreset_cmd_poweroff') +def test_poweroff(u_boot_console): + """Test that the "poweroff" command exits sandbox process.""" - u_boot_console.run_command('reset', wait_for_prompt=False) + u_boot_console.run_command('poweroff', wait_for_prompt=False) assert(u_boot_console.validate_exited()) @pytest.mark.boardspec('sandbox') From c0b19f25a1a2ba935333899e5dcbe4429851cb18 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:24 +0100 Subject: [PATCH 04/44] sandbox: use O_CLOEXEC in os_open() During a cold reset execv() is used to relaunch the U-Boot binary. We must ensure that all files are closed in this case. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/sandbox/cpu/os.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index c461fb0db0..7e474d6364 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -80,6 +80,11 @@ int os_open(const char *pathname, int os_flags) flags |= O_CREAT; if (os_flags & OS_O_TRUNC) flags |= O_TRUNC; + /* + * During a cold reset execv() is used to relaunch the U-Boot binary. + * We must ensure that all files are closed in this case. + */ + flags |= O_CLOEXEC; return open(pathname, flags, 0777); } From 329dccc0675b97d8a1ab9debfb610165262f35c6 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:25 +0100 Subject: [PATCH 05/44] sandbox: implement reset Up to now the sandbox would shutdown upon a cold reset request. Instead it should be reset. In our coding we use static variables like LIST_HEAD(efi_obj_list). A reset can occur at any time, e.g. via an UEFI binary calling the reset service. The only safe way to return to an initial state is to relaunch the U-Boot binary. The reset implementation uses execv() to relaunch U-Boot. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- arch/sandbox/cpu/os.c | 6 ++++++ arch/sandbox/cpu/start.c | 26 +++++++++++++++++++++++ arch/sandbox/cpu/state.c | 1 + arch/sandbox/include/asm/u-boot-sandbox.h | 10 +++++++++ drivers/sysreset/sysreset_sandbox.c | 3 +++ include/os.h | 15 +++++++++++++ 6 files changed, 61 insertions(+) diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c index 7e474d6364..0d8efd83f6 100644 --- a/arch/sandbox/cpu/os.c +++ b/arch/sandbox/cpu/os.c @@ -822,3 +822,9 @@ void *os_find_text_base(void) return base; } + +void os_relaunch(char *argv[]) +{ + execv(argv[0], argv); + os_exit(1); +} diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c index 58ada13fba..a03e5aa0b3 100644 --- a/arch/sandbox/cpu/start.c +++ b/arch/sandbox/cpu/start.c @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -19,6 +20,8 @@ DECLARE_GLOBAL_DATA_PTR; +static char **os_argv; + /* Compare two options so that they can be sorted into alphabetical order */ static int h_compare_opt(const void *p1, const void *p2) { @@ -403,12 +406,35 @@ void state_show(struct sandbox_state *state) printf("\n"); } +void sandbox_reset(void) +{ + /* Do this here while it still has an effect */ + os_fd_restore(); + if (state_uninit()) + os_exit(2); + + if (dm_uninit()) + os_exit(2); + + /* Restart U-Boot */ + os_relaunch(os_argv); +} + int main(int argc, char *argv[]) { struct sandbox_state *state; gd_t data; int ret; + /* + * Copy argv[] so that we can pass the arguments in the original + * sequence when resetting the sandbox. + */ + os_argv = calloc(argc + 1, sizeof(char *)); + if (!os_argv) + os_exit(1); + memcpy(os_argv, argv, sizeof(char *) * (argc + 1)); + memset(&data, '\0', sizeof(data)); gd = &data; gd->arch.text_base = os_find_text_base(); diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c index 34b6fff7e7..59f37fab0b 100644 --- a/arch/sandbox/cpu/state.c +++ b/arch/sandbox/cpu/state.c @@ -358,6 +358,7 @@ void state_reset_for_test(struct sandbox_state *state) /* No reset yet, so mark it as such. Always allow power reset */ state->last_sysreset = SYSRESET_COUNT; state->sysreset_allowed[SYSRESET_POWER_OFF] = true; + state->sysreset_allowed[SYSRESET_COLD] = true; state->allow_memio = false; memset(&state->wdt, '\0', sizeof(state->wdt)); diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h index 798d003077..73b1897191 100644 --- a/arch/sandbox/include/asm/u-boot-sandbox.h +++ b/arch/sandbox/include/asm/u-boot-sandbox.h @@ -84,6 +84,16 @@ void sandbox_set_enable_pci_map(int enable); */ int sandbox_read_fdt_from_file(void); +/** + * sandbox_reset() - reset sandbox + * + * This functions implements the cold reboot of the sandbox. It relaunches the + * U-Boot binary with the same command line parameters as the original call. + * The PID of the process stays the same. All file descriptors that have not + * been opened with O_CLOEXEC stay open including stdin, stdout, stderr. + */ +void sandbox_reset(void); + /* Exit sandbox (quit U-Boot) */ void sandbox_exit(void); diff --git a/drivers/sysreset/sysreset_sandbox.c b/drivers/sysreset/sysreset_sandbox.c index 71cabd1956..7026a48c4b 100644 --- a/drivers/sysreset/sysreset_sandbox.c +++ b/drivers/sysreset/sysreset_sandbox.c @@ -56,6 +56,9 @@ static int sandbox_sysreset_request(struct udevice *dev, enum sysreset_t type) switch (type) { case SYSRESET_COLD: state->last_sysreset = type; + if (!state->sysreset_allowed[type]) + return -EACCES; + sandbox_reset(); break; case SYSRESET_POWER_OFF: state->last_sysreset = type; diff --git a/include/os.h b/include/os.h index 1874ae674f..88dfb71c1a 100644 --- a/include/os.h +++ b/include/os.h @@ -355,4 +355,19 @@ int os_read_file(const char *name, void **bufp, int *sizep); */ void *os_find_text_base(void); +/** + * os_relaunch() - restart the sandbox + * + * This functions is used to implement the cold reboot of the sand box. + * @argv[0] specifies the binary that is started while the calling process + * stops immediately. If the new binary cannot be started, the process is + * terminated and 1 is set as shell return code. + * + * The PID of the process stays the same. All file descriptors that have not + * been opened with O_CLOEXEC stay open including stdin, stdout, stderr. + * + * @argv: NULL terminated list of command line parameters + */ +void os_relaunch(char *argv[]); + #endif From bf896a2f5a8c106c42186ed0a2bb4d2b2d9c6c58 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:26 +0100 Subject: [PATCH 06/44] test: adjust sysreset tests As we have a working COLD_RESET on the sandbox the sysreset test has to be adjusted. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- test/dm/sysreset.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/dm/sysreset.c b/test/dm/sysreset.c index aec97b1cbb..691683c567 100644 --- a/test/dm/sysreset.c +++ b/test/dm/sysreset.c @@ -37,7 +37,9 @@ static int dm_test_sysreset_base(struct unit_test_state *uts) /* Device 2 is the cold sysreset device */ ut_assertok(uclass_get_device(UCLASS_SYSRESET, 2, &dev)); ut_asserteq(-ENOSYS, sysreset_request(dev, SYSRESET_WARM)); + state->sysreset_allowed[SYSRESET_COLD] = false; ut_asserteq(-EACCES, sysreset_request(dev, SYSRESET_COLD)); + state->sysreset_allowed[SYSRESET_COLD] = true; state->sysreset_allowed[SYSRESET_POWER] = false; ut_asserteq(-EACCES, sysreset_request(dev, SYSRESET_POWER)); state->sysreset_allowed[SYSRESET_POWER] = true; @@ -71,22 +73,25 @@ static int dm_test_sysreset_walk(struct unit_test_state *uts) struct sandbox_state *state = state_get_current(); /* If we generate a power sysreset, we will exit sandbox! */ + state->sysreset_allowed[SYSRESET_WARM] = false; + state->sysreset_allowed[SYSRESET_COLD] = false; state->sysreset_allowed[SYSRESET_POWER] = false; state->sysreset_allowed[SYSRESET_POWER_OFF] = false; ut_asserteq(-EACCES, sysreset_walk(SYSRESET_WARM)); ut_asserteq(-EACCES, sysreset_walk(SYSRESET_COLD)); ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER)); + ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER_OFF)); /* * Enable cold system reset - this should make cold system reset work, * plus a warm system reset should be promoted to cold, since this is * the next step along. */ - state->sysreset_allowed[SYSRESET_COLD] = true; + state->sysreset_allowed[SYSRESET_WARM] = true; ut_asserteq(-EINPROGRESS, sysreset_walk(SYSRESET_WARM)); - ut_asserteq(-EINPROGRESS, sysreset_walk(SYSRESET_COLD)); + ut_asserteq(-EACCES, sysreset_walk(SYSRESET_COLD)); ut_asserteq(-EACCES, sysreset_walk(SYSRESET_POWER)); - state->sysreset_allowed[SYSRESET_COLD] = false; + state->sysreset_allowed[SYSRESET_COLD] = true; state->sysreset_allowed[SYSRESET_POWER] = true; return 0; From 063790cb62c90b712d298d6d21d910d1a52492b5 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:27 +0100 Subject: [PATCH 07/44] sandbox: update function descriptions in os.h Use Sphinx style function descriptions. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- include/os.h | 223 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 130 insertions(+), 93 deletions(-) diff --git a/include/os.h b/include/os.h index 88dfb71c1a..1fe44f3510 100644 --- a/include/os.h +++ b/include/os.h @@ -19,30 +19,30 @@ struct sandbox_state; /** * Access to the OS read() system call * - * \param fd File descriptor as returned by os_open() - * \param buf Buffer to place data - * \param count Number of bytes to read - * \return number of bytes read, or -1 on error + * @fd: File descriptor as returned by os_open() + * @buf: Buffer to place data + * @count: Number of bytes to read + * Return: number of bytes read, or -1 on error */ ssize_t os_read(int fd, void *buf, size_t count); /** * Access to the OS write() system call * - * \param fd File descriptor as returned by os_open() - * \param buf Buffer containing data to write - * \param count Number of bytes to write - * \return number of bytes written, or -1 on error + * @fd: File descriptor as returned by os_open() + * @buf: Buffer containing data to write + * @count: Number of bytes to write + * Return: number of bytes written, or -1 on error */ ssize_t os_write(int fd, const void *buf, size_t count); /** * Access to the OS lseek() system call * - * \param fd File descriptor as returned by os_open() - * \param offset File offset (based on whence) - * \param whence Position offset is relative to (see below) - * \return new file offset + * @fd: File descriptor as returned by os_open() + * @offset: File offset (based on whence) + * @whence: Position offset is relative to (see below) + * Return: new file offset */ off_t os_lseek(int fd, off_t offset, int whence); @@ -54,9 +54,9 @@ off_t os_lseek(int fd, off_t offset, int whence); /** * Access to the OS open() system call * - * \param pathname Pathname of file to open - * \param flags Flags, like OS_O_RDONLY, OS_O_RDWR - * \return file descriptor, or -1 on error + * @pathname: Pathname of file to open + * @flags: Flags, like OS_O_RDONLY, OS_O_RDWR + * Return: file descriptor, or -1 on error */ int os_open(const char *pathname, int flags); @@ -68,42 +68,42 @@ int os_open(const char *pathname, int flags); #define OS_O_TRUNC 01000 /** - * Access to the OS close() system call + * os_close() - access to the OS close() system call * - * \param fd File descriptor to close - * \return 0 on success, -1 on error + * @fd: File descriptor to close + * Return: 0 on success, -1 on error */ int os_close(int fd); /** - * Access to the OS unlink() system call + * os_unlink() - access to the OS unlink() system call * - * \param pathname Path of file to delete - * \return 0 for success, other for error + * @pathname: Path of file to delete + * Return: 0 for success, other for error */ int os_unlink(const char *pathname); /** - * Access to the OS exit() system call + * os_exit() - access to the OS exit() system call * * This exits with the supplied return code, which should be 0 to indicate * success. * - * @param exit_code exit code for U-Boot + * @exit_code: exit code for U-Boot */ void os_exit(int exit_code) __attribute__((noreturn)); /** - * Put tty into raw mode to mimic serial console better + * os_tty_raw() - put tty into raw mode to mimic serial console better * - * @param fd File descriptor of stdin (normally 0) - * @param allow_sigs Allow Ctrl-C, Ctrl-Z to generate signals rather than - * be handled by U-Boot + * @fd: File descriptor of stdin (normally 0) + * @allow_sigs: Allow Ctrl-C, Ctrl-Z to generate signals rather than + * be handled by U-Boot */ void os_tty_raw(int fd, bool allow_sigs); /** - * Restore the tty to its original mode + * os_fs_restore() - restore the tty to its original mode * * Call this to restore the original terminal mode, after it has been changed * by os_tty_raw(). This is an internal function. @@ -111,144 +111,180 @@ void os_tty_raw(int fd, bool allow_sigs); void os_fd_restore(void); /** - * Acquires some memory from the underlying os. + * os_malloc() - aquires some memory from the underlying os. * - * \param length Number of bytes to be allocated - * \return Pointer to length bytes or NULL on error + * @length: Number of bytes to be allocated + * Return: Pointer to length bytes or NULL on error */ void *os_malloc(size_t length); /** - * Free memory previous allocated with os_malloc() + * os_free() - free memory previous allocated with os_malloc() * * This returns the memory to the OS. * - * \param ptr Pointer to memory block to free + * @ptr: Pointer to memory block to free */ void os_free(void *ptr); /** - * Access to the usleep function of the os + * os_usleep() - access to the usleep function of the os * - * \param usec Time to sleep in micro seconds + * @usec: time to sleep in micro seconds */ void os_usleep(unsigned long usec); /** * Gets a monotonic increasing number of nano seconds from the OS * - * \return A monotonic increasing time scaled in nano seconds + * Return: a monotonic increasing time scaled in nano seconds */ uint64_t os_get_nsec(void); /** * Parse arguments and update sandbox state. * - * @param state Sandbox state to update - * @param argc Argument count - * @param argv Argument vector - * @return 0 if ok, and program should continue; - * 1 if ok, but program should stop; - * -1 on error: program should terminate. + * @state: sandbox state to update + * @argc: argument count + * @argv: argument vector + * Return: + * * 0 if ok, and program should continue + * * 1 if ok, but program should stop + * * -1 on error: program should terminate */ int os_parse_args(struct sandbox_state *state, int argc, char *argv[]); /* + * enum os_dirent_t - type of directory entry + * * Types of directory entry that we support. See also os_dirent_typename in * the C file. */ enum os_dirent_t { - OS_FILET_REG, /* Regular file */ - OS_FILET_LNK, /* Symbolic link */ - OS_FILET_DIR, /* Directory */ - OS_FILET_UNKNOWN, /* Something else */ - + /** + * @OS_FILET_REG: regular file + */ + OS_FILET_REG, + /** + * @OS_FILET_LNK: symbolic link + */ + OS_FILET_LNK, + /** + * @OS_FILET_DIR: directory + */ + OS_FILET_DIR, + /** + * @OS_FILET_UNKNOWN: something else + */ + OS_FILET_UNKNOWN, + /** + * @OS_FILET_COUNT: number of directory entry types + */ OS_FILET_COUNT, }; -/** A directory entry node, containing information about a single dirent */ +/** + * struct os_dirent_node - directory node + * + * A directory entry node, containing information about a single dirent + * + */ struct os_dirent_node { - struct os_dirent_node *next; /* Pointer to next node, or NULL */ - ulong size; /* Size of file in bytes */ - enum os_dirent_t type; /* Type of entry */ - char name[0]; /* Name of entry */ + /** + * @next: pointer to next node, or NULL + */ + struct os_dirent_node *next; + /** + * @size: size of file in bytes + */ + ulong size; + /** + * @type: type of entry + */ + enum os_dirent_t type; + /** + * @name: name of entry + */ + char name[0]; }; /** - * Get a directionry listing + * os_dirent_ls() - get a directory listing * * This allocates and returns a linked list containing the directory listing. * - * @param dirname Directory to examine - * @param headp Returns pointer to head of linked list, or NULL if none - * @return 0 if ok, -ve on error + * @dirname: directory to examine + * @headp: on return pointer to head of linked list, or NULL if none + * Return: 0 if ok, -ve on error */ int os_dirent_ls(const char *dirname, struct os_dirent_node **headp); /** - * Free directory list + * os_dirent_free() - free directory list * * This frees a linked list containing a directory listing. * - * @param node Pointer to head of linked list + * @node: pointer to head of linked list */ void os_dirent_free(struct os_dirent_node *node); /** - * Get the name of a directory entry type + * os_dirent_get_typename() - get the name of a directory entry type * - * @param type Type to check - * @return string containing the name of that type, or "???" if none/invalid + * @type: type to check + * Return: + * string containing the name of that type, + * or "???" if none/invalid */ const char *os_dirent_get_typename(enum os_dirent_t type); /** - * Get the size of a file + * os_get_filesize() - get the size of a file * - * @param fname Filename to check - * @param size size of file is returned if no error - * @return 0 on success or -1 if an error ocurred + * @fname: filename to check + * @size: size of file is returned if no error + * Return: 0 on success or -1 if an error ocurred */ int os_get_filesize(const char *fname, loff_t *size); /** - * Write a character to the controlling OS terminal + * os_putc() - write a character to the controlling OS terminal * * This bypasses the U-Boot console support and writes directly to the OS * stdout file descriptor. * - * @param ch Character to write + * @ch: haracter to write */ void os_putc(int ch); /** - * Write a string to the controlling OS terminal + * os_puts() - write a string to the controlling OS terminal * * This bypasses the U-Boot console support and writes directly to the OS * stdout file descriptor. * - * @param str String to write (note that \n is not appended) + * @str: string to write (note that \n is not appended) */ void os_puts(const char *str); /** - * Write the sandbox RAM buffer to a existing file + * os_write_ram_buf() - write the sandbox RAM buffer to a existing file * - * @param fname Filename to write memory to (simple binary format) - * @return 0 if OK, -ve on error + * @fname: filename to write memory to (simple binary format) + * Return: 0 if OK, -ve on error */ int os_write_ram_buf(const char *fname); /** - * Read the sandbox RAM buffer from an existing file + * os_read_ram_buf() - read the sandbox RAM buffer from an existing file * - * @param fname Filename containing memory (simple binary format) - * @return 0 if OK, -ve on error + * @fname: filename containing memory (simple binary format) + * Return: 0 if OK, -ve on error */ int os_read_ram_buf(const char *fname); /** - * Jump to a new executable image + * os_jump_to_image() - jump to a new executable image * * This uses exec() to run a new executable image, after putting it in a * temporary file. The same arguments and environment are passed to this @@ -261,22 +297,23 @@ int os_read_ram_buf(const char *fname); * have access to this. It also means that the original * memory filename passed to U-Boot will be left intact. * - * @param dest Buffer containing executable image - * @param size Size of buffer + * @dest: buffer containing executable image + * @size: size of buffer + * Return: 0 if OK, -ve on error */ int os_jump_to_image(const void *dest, int size); /** - * os_find_u_boot() - Determine the path to U-Boot proper + * os_find_u_boot() - determine the path to U-Boot proper * * This function is intended to be called from within sandbox SPL. It uses * a few heuristics to find U-Boot proper. Normally it is either in the same * directory, or the directory above (since u-boot-spl is normally in an * spl/ subdirectory when built). * - * @fname: Place to put full path to U-Boot - * @maxlen: Maximum size of @fname - * @return 0 if OK, -NOSPC if the filename is too large, -ENOENT if not found + * @fname: place to put full path to U-Boot + * @maxlen: maximum size of @fname + * Return: 0 if OK, -NOSPC if the filename is too large, -ENOENT if not found */ int os_find_u_boot(char *fname, int maxlen); @@ -286,23 +323,23 @@ int os_find_u_boot(char *fname, int maxlen); * When called from SPL, this runs U-Boot proper. The filename is obtained by * calling os_find_u_boot(). * - * @fname: Full pathname to U-Boot executable - * @return 0 if OK, -ve on error + * @fname: full pathname to U-Boot executable + * Return: 0 if OK, -ve on error */ int os_spl_to_uboot(const char *fname); /** - * Read the current system time + * os_localtime() - read the current system time * * This reads the current Local Time and places it into the provided * structure. * - * @param rt Place to put system time + * @rt: place to put system time */ void os_localtime(struct rtc_time *rt); /** - * os_abort() - Raise SIGABRT to exit sandbox (e.g. to debugger) + * os_abort() - raise SIGABRT to exit sandbox (e.g. to debugger) */ void os_abort(void); @@ -313,12 +350,12 @@ void os_abort(void); * * @start: Region start * @len: Region length in bytes - * @return 0 if OK, -1 on error from mprotect() + * Return: 0 if OK, -1 on error from mprotect() */ int os_mprotect_allow(void *start, size_t len); /** - * os_write_file() - Write a file to the host filesystem + * os_write_file() - write a file to the host filesystem * * This can be useful when debugging for writing data out of sandbox for * inspection by external tools. @@ -326,7 +363,7 @@ int os_mprotect_allow(void *start, size_t len); * @name: File path to write to * @buf: Data to write * @size: Size of data to write - * @return 0 if OK, -ve on error + * Return: 0 if OK, -ve on error */ int os_write_file(const char *name, const void *buf, int size); @@ -340,7 +377,7 @@ int os_write_file(const char *name, const void *buf, int size); * @name: File path to read from * @bufp: Returns buffer containing data read * @sizep: Returns size of data - * @return 0 if OK, -ve on error + * Return: 0 if OK, -ve on error */ int os_read_file(const char *name, void **bufp, int *sizep); @@ -351,7 +388,7 @@ int os_read_file(const char *name, void **bufp, int *sizep); * It can be useful to map the address of functions to the address listed in * the u-boot.map file. * - * @return address if found, else NULL + * Return: address if found, else NULL */ void *os_find_text_base(void); @@ -359,7 +396,7 @@ void *os_find_text_base(void); * os_relaunch() - restart the sandbox * * This functions is used to implement the cold reboot of the sand box. - * @argv[0] specifies the binary that is started while the calling process + * @argv\[0] specifies the binary that is started while the calling process * stops immediately. If the new binary cannot be started, the process is * terminated and 1 is set as shell return code. * From 0ae9bc3c42f1e47ac4954bc3b3112067e15bb6f3 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 27 Oct 2020 20:29:28 +0100 Subject: [PATCH 08/44] doc: add sandbox API Add sandbox API to generated HTML documentation Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- doc/api/index.rst | 1 + doc/api/sandbox.rst | 9 +++++++++ 2 files changed, 10 insertions(+) create mode 100644 doc/api/sandbox.rst diff --git a/doc/api/index.rst b/doc/api/index.rst index ae4a1b6c63..cbecd10755 100644 --- a/doc/api/index.rst +++ b/doc/api/index.rst @@ -12,6 +12,7 @@ U-Boot API documentation linker_lists pinctrl rng + sandbox serial timer unicode diff --git a/doc/api/sandbox.rst b/doc/api/sandbox.rst new file mode 100644 index 0000000000..724776399b --- /dev/null +++ b/doc/api/sandbox.rst @@ -0,0 +1,9 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +Sandbox +======= + +The following API routines are used to implement the U-Boot sandbox. + +.. kernel-doc:: include/os.h + :internal: From 6723b4c6ca7b6ef1435ee5529a53e14df3e1b099 Mon Sep 17 00:00:00 2001 From: Michal Simek Date: Wed, 26 Aug 2020 15:13:14 +0200 Subject: [PATCH 09/44] binman: Call helper function binman_set_rom_offset() to fill offset There is prepared function for filing rom_offset. That's why use it instead of copying content of it. Signed-off-by: Michal Simek Reviewed-by: Simon Glass Reviewed-by: Simon Glass --- lib/binman.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/binman.c b/lib/binman.c index e71c1b9e99..d395b1cf70 100644 --- a/lib/binman.c +++ b/lib/binman.c @@ -103,7 +103,7 @@ int binman_init(void) return log_msg_ret("first image", -ENOENT); binman->image = node; } - binman->rom_offset = ROM_OFFSET_NONE; - + binman_set_rom_offset(ROM_OFFSET_NONE); +\ return 0; } From 9485a80be1354e55fc712ba82817dbe8779284d8 Mon Sep 17 00:00:00 2001 From: Michal Simek Date: Wed, 26 Aug 2020 15:34:24 +0200 Subject: [PATCH 10/44] binman: Fix typo in kernel-doc format for binman_symname() Fix typo. Fixes: 19790632648b ("binman: Support accessing binman tables at run time") Signed-off-by: Michal Simek Reviewed-by: Simon Glass Reviewed-by: Simon Glass --- include/binman_sym.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/binman_sym.h b/include/binman_sym.h index ffb7c15798..72e6765fe5 100644 --- a/include/binman_sym.h +++ b/include/binman_sym.h @@ -16,7 +16,7 @@ #ifdef CONFIG_BINMAN /** - * binman_symname() - Internal fnuction to get a binman symbol name + * binman_symname() - Internal function to get a binman symbol name * * @entry_name: Name of the entry to look for (e.g. 'u_boot_spl') * @_prop_name: Property value to get from that entry (e.g. 'pos') From 690079767803c7406062dc074bbb1f8f0ff37fa0 Mon Sep 17 00:00:00 2001 From: Alper Nebi Yasak Date: Fri, 30 Oct 2020 20:25:20 +0300 Subject: [PATCH 11/44] cros_ec: Support keyboard scanning with EC_CMD_GET_NEXT_EVENT The cros_ec_keyb driver currently uses EC_CMD_MKBP_STATE to scan the keyboard, but this host command was superseded by EC_CMD_GET_NEXT_EVENT and unavailable on more recent devices (including gru-kevin), as it was removed in cros-ec commit 87a071941b89 ("mkbp: Add support for buttons and switches.") dated 2016-07-06. The EC_CMD_GET_NEXT_EVENT has been available since cros-ec commit d1ed75815efe ("MKBP event signalling implementation") dated 2014-10-20, but it looks like it isn't included in firmware-* branches for at least link, nyan-big, samus, snow, spring, panther and peach-pit which have defconfigs in U-Boot. So this patch falls back to the old method if the EC doesn't recognize the newer command. The implementation is mostly adapted from Depthcharge commit f88af26b44fc ("cros_ec: Change keyboard scanning method."). On a gru-kevin, the current driver before this patch fails to read the pressed keys with: out: cmd=0x60: 03 9d 60 00 00 00 00 00 in-header: 03 fc 01 00 00 00 00 00 in-data: ec_command_inptr: len=-1, din=0000000000000000 check_for_keys: keyboard scan failed However the keyboard works fine with the newer command: out: cmd=0x67: 03 96 67 00 00 00 00 00 in-header: 03 ef 00 00 0e 00 00 00 in-data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ec_command_inptr: len=14, din=00000000f412df30 key_matrix_decode: num_keys = 0 0 valid keycodes found out: cmd=0x67: 03 96 67 00 00 00 00 00 in-header: 03 df 00 00 0e 00 00 00 in-data: 00 00 00 00 00 00 00 00 00 00 00 00 10 00 ec_command_inptr: len=14, din=00000000f412df30 key_matrix_decode: num_keys = 1 valid=1, row=4, col=11 keycode=28 1 valid keycodes found {0d} Signed-off-by: Alper Nebi Yasak Reviewed-by: Simon Glass --- drivers/input/cros_ec_keyb.c | 32 ++++++++++++++++++++++++++------ drivers/misc/cros_ec.c | 15 +++++++++++++++ include/cros_ec.h | 11 +++++++++++ 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/drivers/input/cros_ec_keyb.c b/drivers/input/cros_ec_keyb.c index 00bf58f2b5..0c0f52205b 100644 --- a/drivers/input/cros_ec_keyb.c +++ b/drivers/input/cros_ec_keyb.c @@ -47,15 +47,35 @@ static int check_for_keys(struct udevice *dev, struct key_matrix_key *keys, struct key_matrix_key *key; static struct mbkp_keyscan last_scan; static bool last_scan_valid; - struct mbkp_keyscan scan; + struct ec_response_get_next_event event; + struct mbkp_keyscan *scan = (struct mbkp_keyscan *) + &event.data.key_matrix; unsigned int row, col, bit, data; int num_keys; + int ret; - if (cros_ec_scan_keyboard(dev->parent, &scan)) { - debug("%s: keyboard scan failed\n", __func__); + /* Get pending MKBP event. It may not be a key matrix event. */ + do { + ret = cros_ec_get_next_event(dev->parent, &event); + /* The EC has no events for us at this time. */ + if (ret == -EC_RES_UNAVAILABLE) + return -EIO; + else if (ret) + break; + } while (event.event_type != EC_MKBP_EVENT_KEY_MATRIX); + + /* Try the old command if the EC doesn't support the above. */ + if (ret == -EC_RES_INVALID_COMMAND) { + if (cros_ec_scan_keyboard(dev->parent, scan)) { + debug("%s: keyboard scan failed\n", __func__); + return -EIO; + } + } else if (ret) { + debug("%s: Error getting next MKBP event. (%d)\n", + __func__, ret); return -EIO; } - *samep = last_scan_valid && !memcmp(&last_scan, &scan, sizeof(scan)); + *samep = last_scan_valid && !memcmp(&last_scan, scan, sizeof(*scan)); /* * This is a bit odd. The EC has no way to tell us that it has run @@ -64,14 +84,14 @@ static int check_for_keys(struct udevice *dev, struct key_matrix_key *keys, * that this scan is the same as the last. */ last_scan_valid = true; - memcpy(&last_scan, &scan, sizeof(last_scan)); + memcpy(&last_scan, scan, sizeof(last_scan)); for (col = num_keys = bit = 0; col < priv->matrix.num_cols; col++) { for (row = 0; row < priv->matrix.num_rows; row++) { unsigned int mask = 1 << (bit & 7); - data = scan.data[bit / 8]; + data = scan->data[bit / 8]; if ((data & mask) && num_keys < max_count) { key = keys + num_keys++; key->row = row; diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c index a5534b1667..c3674908ee 100644 --- a/drivers/misc/cros_ec.c +++ b/drivers/misc/cros_ec.c @@ -415,6 +415,21 @@ int cros_ec_scan_keyboard(struct udevice *dev, struct mbkp_keyscan *scan) return 0; } +int cros_ec_get_next_event(struct udevice *dev, + struct ec_response_get_next_event *event) +{ + int ret; + + ret = ec_command(dev, EC_CMD_GET_NEXT_EVENT, 0, NULL, 0, + event, sizeof(*event)); + if (ret < 0) + return ret; + else if (ret != sizeof(*event)) + return -EC_RES_INVALID_RESPONSE; + + return 0; +} + int cros_ec_read_id(struct udevice *dev, char *id, int maxlen) { struct ec_response_get_version *r; diff --git a/include/cros_ec.h b/include/cros_ec.h index f4b9b7a5c2..f187bd0d4b 100644 --- a/include/cros_ec.h +++ b/include/cros_ec.h @@ -82,6 +82,17 @@ int cros_ec_read_id(struct udevice *dev, char *id, int maxlen); */ int cros_ec_scan_keyboard(struct udevice *dev, struct mbkp_keyscan *scan); +/** + * Get the next pending MKBP event from the ChromeOS EC device. + * + * Send a message requesting the next event and return the result. + * + * @param event Place to put the event. + * @return 0 if ok, <0 on error. + */ +int cros_ec_get_next_event(struct udevice *dev, + struct ec_response_get_next_event *event); + /** * Read which image is currently running on the CROS-EC device. * From c961b1b594df3a9099afb425d46e820e07dfc315 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 31 Oct 2020 08:38:06 +0100 Subject: [PATCH 12/44] test: test/bloblist.c depends on asm/state.h Building test/bloblist.c fails for non sandbox devices: test/bloblist.c:10:10: fatal error: asm/state.h: No such file or directory #include ^~~~~~~~~~~~~ Build the test only on the sandbox. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- test/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/Makefile b/test/Makefile index 1c930b3148..39ae04a3d2 100644 --- a/test/Makefile +++ b/test/Makefile @@ -2,7 +2,9 @@ # # (C) Copyright 2012 The Chromium Authors +ifneq ($(CONFIG_SANDBOX),) obj-$(CONFIG_$(SPL_)CMDLINE) += bloblist.o +endif obj-$(CONFIG_$(SPL_)CMDLINE) += cmd/ obj-$(CONFIG_$(SPL_)CMDLINE) += cmd_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += command_ut.o From de429d7b0962a91f91bbe921600745fb4608ece0 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 31 Oct 2020 08:59:25 +0100 Subject: [PATCH 13/44] test: linking test/compression.c fails Building U-Boot with unit tests on a non-sandbox systems fails: ld.bfd: test/built-in.o: in function `compress_using_gzip': test/compression.c:138: undefined reference to `gzip' ld.bfd: test/built-in.o: in function `uncompress_using_bzip2': test/compression.c:187: undefined reference to `BZ2_bzBuffToBuffDecompress' ld.bfd: test/built-in.o: in function `uncompress_using_lzma': test/compression.c:222: undefined reference to `lzmaBuffToBuffDecompress' ld.bfd: test/built-in.o: in function `uncompress_using_lzo': test/compression.c:257: undefined reference to `lzop_decompress' ld.bfd: test/built-in.o: in function `uncompress_using_lz4': test/compression.c:292: undefined reference to `ulz4fn Add the missing dependencies. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- test/Kconfig | 9 +++++++++ test/Makefile | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/test/Kconfig b/test/Kconfig index 2646e7d825..ab3ac54a1b 100644 --- a/test/Kconfig +++ b/test/Kconfig @@ -50,6 +50,15 @@ config UT_LIB_RSA endif +config UT_COMPRESSION + bool "Unit test for compression" + depends on UNIT_TEST + depends on CMDLINE && GZIP_COMPRESSED && BZIP2 && LZMA && LZO && LZ4 + default y + help + Enables tests for compression and decompression routines for simple + sanity and for buffer overflow conditions. + config UT_LOG bool "Unit tests for logging functions" depends on UNIT_TEST diff --git a/test/Makefile b/test/Makefile index 39ae04a3d2..8296734eb3 100644 --- a/test/Makefile +++ b/test/Makefile @@ -8,7 +8,7 @@ endif obj-$(CONFIG_$(SPL_)CMDLINE) += cmd/ obj-$(CONFIG_$(SPL_)CMDLINE) += cmd_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += command_ut.o -obj-$(CONFIG_$(SPL_)CMDLINE) += compression.o +obj-$(CONFIG_$(SPL_)UT_COMPRESSION) += compression.o obj-y += dm/ obj-$(CONFIG_$(SPL_)CMDLINE) += print_ut.o obj-$(CONFIG_$(SPL_)CMDLINE) += str_ut.o From 271a08380634f2def4a6b148187cce6693e878a5 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 2 Nov 2020 12:55:43 -0700 Subject: [PATCH 14/44] binman: Update intel_ifwi to store padded section With a recent change this entry stores only part of the section data, leaving out the padding at the end. Fix this by using GetPaddedData() to get the data. Add this function to the base Entry class also. Fixes: d1d3ad7d1fe ("binman: Move section padding to the parent") Signed-off-by: Simon Glass --- tools/binman/entry.py | 16 ++++++++++++++++ tools/binman/etype/intel_ifwi.py | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 8946d2bc02..68e694e7d1 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -456,6 +456,22 @@ class Entry(object): self.Detail('GetData: size %s' % ToHexSize(self.data)) return self.data + def GetPaddedData(self, data=None): + """Get the data for an entry including any padding + + Gets the entry data and uses its section's pad-byte value to add padding + before and after as defined by the pad-before and pad-after properties. + + This does not consider alignment. + + Returns: + Contents of the entry along with any pad bytes before and + after it (bytes) + """ + if data is None: + data = self.GetData() + return self.section.GetPaddedDataForEntry(self, data) + def GetOffsets(self): """Get the offsets for siblings diff --git a/tools/binman/etype/intel_ifwi.py b/tools/binman/etype/intel_ifwi.py index 76b3357c25..1a0e481c19 100644 --- a/tools/binman/etype/intel_ifwi.py +++ b/tools/binman/etype/intel_ifwi.py @@ -71,7 +71,7 @@ class Entry_intel_ifwi(Entry_blob_ext): for entry in self._ifwi_entries.values(): # First get the input data and put it in a file - data = entry.GetData() + data = entry.GetPaddedData() uniq = self.GetUniqueName() input_fname = tools.GetOutputFilename('input.%s' % uniq) tools.WriteFile(input_fname, data) From 08594d49860c335f39d4f7797cac766ac8b57c56 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Mon, 2 Nov 2020 12:55:44 -0700 Subject: [PATCH 15/44] binman: Correct calculation for image-pos A recent change removed the base offset from the calculation. This is used on coral to find the FSP-S binary. Fix it. Fixes: a9fad07d4b8 ("binman: Avoid reporting image-pos with compression") Signed-off-by: Simon Glass --- tools/binman/entry.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/binman/entry.py b/tools/binman/entry.py index 68e694e7d1..2be0d8e053 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -245,7 +245,7 @@ class Entry(object): state.SetInt(self._node, 'size', self.size) base = self.section.GetRootSkipAtStart() if self.section else 0 if self.image_pos is not None: - state.SetInt(self._node, 'image-pos', self.image_pos) + state.SetInt(self._node, 'image-pos', self.image_pos - base) if self.GetImage().allow_repack: if self.orig_offset is not None: state.SetInt(self._node, 'orig-offset', self.orig_offset, True) From 4a9e578138c60aa22821910e22a6e6289c884363 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:10 -0600 Subject: [PATCH 16/44] patman: Correct operation of -n This operation was unfortunately broken by a recent change. It is now necessary to use -i in addition to -n, if there are errors or warnings in the patches. Correct this by always showing the summary information. Fixes: f3653759758 ("patman: Move main code out to a control module") Signed-off-by: Simon Glass --- tools/patman/control.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tools/patman/control.py b/tools/patman/control.py index 67e8f397ef..aea9df8c8d 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -170,9 +170,8 @@ def send(args): ok = ok and gitutil.CheckSuppressCCConfig() its_a_go = ok or args.ignore_errors - if its_a_go: - email_patches( - col, series, cover_fname, patch_files, args.process_tags, - its_a_go, args.ignore_bad_tags, args.add_maintainers, - args.limit, args.dry_run, args.in_reply_to, args.thread, - args.smtp_server) + email_patches( + col, series, cover_fname, patch_files, args.process_tags, + its_a_go, args.ignore_bad_tags, args.add_maintainers, + args.limit, args.dry_run, args.in_reply_to, args.thread, + args.smtp_server) From bd73bb447ffc11f0fbc567e9edc785f98f5ab673 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:11 -0600 Subject: [PATCH 17/44] azure/gitLab/travis: Add pygit2 as a dependency for tests This lets patman run all of its tests, rather than skipping quite a few. Signed-off-by: Simon Glass --- .azure-pipelines.yml | 2 +- .gitlab-ci.yml | 2 +- .travis.yml | 1 + 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml index a78c8d6130..620696c22e 100644 --- a/.azure-pipelines.yml +++ b/.azure-pipelines.yml @@ -140,7 +140,7 @@ jobs: export USER=azure virtualenv -p /usr/bin/python3 /tmp/venv . /tmp/venv/bin/activate - pip install pyelftools pytest + pip install pyelftools pytest pygit2 export UBOOT_TRAVIS_BUILD_DIR=/tmp/sandbox_spl export PYTHONPATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt export PATH=${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH} diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b1e0b3bc9d..4b0680887b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -161,7 +161,7 @@ Run binman, buildman, dtoc, Kconfig and patman testsuites: export USER=gitlab; virtualenv -p /usr/bin/python3 /tmp/venv; . /tmp/venv/bin/activate; - pip install pyelftools pytest; + pip install pyelftools pytest pygit2; export UBOOT_TRAVIS_BUILD_DIR=/tmp/sandbox_spl; export PYTHONPATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc/pylibfdt"; export PATH="${UBOOT_TRAVIS_BUILD_DIR}/scripts/dtc:${PATH}"; diff --git a/.travis.yml b/.travis.yml index cb48ff3023..69829fcfa0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -26,6 +26,7 @@ addons: - python3-sphinx - python3-virtualenv - python3-pip + - python3-pygit2 - swig - libpython-dev - iasl From e21c515817eacc49422d9bb651894a2e724414dc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:12 -0600 Subject: [PATCH 18/44] patman: Update how tests are run The current instructions are out-of-date. Fix them. Signed-off-by: Simon Glass --- tools/patman/README | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/patman/README b/tools/patman/README index 52b2cf70bd..7ede1068ee 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -533,12 +533,10 @@ Most of these are indicated by a TODO in the code. It would be nice if this could handle the In-reply-to side of things. -The tests are incomplete, as is customary. Use the --test flag to run them, -and make sure you are in the tools/patman directory first: +The tests are incomplete, as is customary. Use the 'test' subcommand to run +them: - $ cd /path/to/u-boot - $ cd tools/patman - $ ./patman --test + $ tools/patman/patman test Error handling doesn't always produce friendly error messages - e.g. putting an incorrect tag in a commit may provide a confusing message. From 427b028aeb2f3678aced5568a5d86c2fff406f46 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:13 -0600 Subject: [PATCH 19/44] patman: Fix whitespace errors in func_test Fix up various indentation and other minor things to make pylint3 happier. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 810af9c604..7fffe64997 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -24,20 +24,20 @@ from patman.test_util import capture_sys_output try: import pygit2 - HAVE_PYGIT2= True + HAVE_PYGIT2 = True except ModuleNotFoundError: HAVE_PYGIT2 = False @contextlib.contextmanager def capture(): - oldout,olderr = sys.stdout, sys.stderr + oldout, olderr = sys.stdout, sys.stderr try: - out=[StringIO(), StringIO()] - sys.stdout,sys.stderr = out + out = [StringIO(), StringIO()] + sys.stdout, sys.stderr = out yield out finally: - sys.stdout,sys.stderr = oldout, olderr + sys.stdout, sys.stderr = oldout, olderr out[0] = out[0].getvalue() out[1] = out[1].getvalue() @@ -160,10 +160,10 @@ class TestFunctional(unittest.TestCase): in_reply_to = mel count = 2 settings.alias = { - 'fdt': ['simon'], - 'u-boot': ['u-boot@lists.denx.de'], - 'simon': [ed], - 'fred': [fred], + 'fdt': ['simon'], + 'u-boot': ['u-boot@lists.denx.de'], + 'simon': [ed], + 'fred': [fred], } text = self.GetText('test01.txt') @@ -177,9 +177,9 @@ class TestFunctional(unittest.TestCase): cc_file = series.MakeCcFile(process_tags, cover_fname, not ignore_bad_tags, add_maintainers, None) - cmd = gitutil.EmailPatches(series, cover_fname, args, - dry_run, not ignore_bad_tags, cc_file, - in_reply_to=in_reply_to, thread=None) + cmd = gitutil.EmailPatches( + series, cover_fname, args, dry_run, not ignore_bad_tags, + cc_file, in_reply_to=in_reply_to, thread=None) series.ShowActions(args, cmd, process_tags) cc_lines = open(cc_file, encoding='utf-8').read().splitlines() os.remove(cc_file) @@ -221,8 +221,9 @@ class TestFunctional(unittest.TestCase): self.assertEqual(('%s %s\0%s' % (args[0], rick, stefan)), tools.ToUnicode(cc_lines[0])) - self.assertEqual(('%s %s\0%s\0%s\0%s' % (args[1], fred, ed, rick, - stefan)), tools.ToUnicode(cc_lines[1])) + self.assertEqual( + '%s %s\0%s\0%s\0%s' % (args[1], fred, ed, rick, stefan), + tools.ToUnicode(cc_lines[1])) expected = ''' This is a test of how the cover @@ -256,8 +257,8 @@ Simon Glass (2): ''' lines = open(cover_fname, encoding='utf-8').read().splitlines() self.assertEqual( - 'Subject: [RFC PATCH v3 0/2] test: A test patch series', - lines[3]) + 'Subject: [RFC PATCH v3 0/2] test: A test patch series', + lines[3]) self.assertEqual(expected.splitlines(), lines[7:]) for i, fname in enumerate(args): @@ -310,7 +311,7 @@ Changes in v2: tools.WriteFile(path, text, binary=False) index = self.repo.index index.add(fname) - author = pygit2.Signature('Test user', 'test@email.com') + author = pygit2.Signature('Test user', 'test@email.com') committer = author tree = index.write_tree() message = subject + '\n' + body @@ -335,7 +336,7 @@ Changes in v2: author = pygit2.Signature('Test user', 'test@email.com') committer = author commit = repo.create_commit('HEAD', author, committer, - 'Created master', new_tree, []) + 'Created master', new_tree, []) self.make_commit_with_file('Initial commit', ''' Add a README From 366954fdd75200852c134003de4a66b0a20f9362 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:14 -0600 Subject: [PATCH 20/44] patman: Use capture_sys_output() consistently One test still uses its own function for capturing output. Modify it to use the standard one in test_util Signed-off-by: Simon Glass --- tools/patman/func_test.py | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 7fffe64997..263cb340ef 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -4,7 +4,6 @@ # Copyright 2017 Google, Inc # -import contextlib import os import re import shutil @@ -12,8 +11,6 @@ import sys import tempfile import unittest -from io import StringIO - from patman import control from patman import gitutil from patman import patchstream @@ -29,19 +26,6 @@ except ModuleNotFoundError: HAVE_PYGIT2 = False -@contextlib.contextmanager -def capture(): - oldout, olderr = sys.stdout, sys.stderr - try: - out = [StringIO(), StringIO()] - sys.stdout, sys.stderr = out - yield out - finally: - sys.stdout, sys.stderr = oldout, olderr - out[0] = out[0].getvalue() - out[1] = out[1].getvalue() - - class TestFunctional(unittest.TestCase): def setUp(self): self.tmpdir = tempfile.mkdtemp(prefix='patman.') @@ -169,7 +153,7 @@ class TestFunctional(unittest.TestCase): text = self.GetText('test01.txt') series = patchstream.GetMetaDataForTest(text) cover_fname, args = self.CreatePatchesForTest(series) - with capture() as out: + with capture_sys_output() as out: patchstream.FixPatches(series, args) if cover_fname and series.get('cover'): patchstream.InsertCoverLetter(cover_fname, series, count) @@ -184,7 +168,7 @@ class TestFunctional(unittest.TestCase): cc_lines = open(cc_file, encoding='utf-8').read().splitlines() os.remove(cc_file) - lines = out[0].splitlines() + lines = out[0].getvalue().splitlines() self.assertEqual('Cleaned %s patches' % len(series.commits), lines[0]) self.assertEqual('Change log missing for v2', lines[1]) self.assertEqual('Change log missing for v3', lines[2]) From fca99117a5512ad0af87fabef3954dce353d765f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:15 -0600 Subject: [PATCH 21/44] patman: Fix remaining pylint3 warnings in func_test This fixes all but the ones about too many variables/statements. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 76 +++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 263cb340ef..b3c3e5796a 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -4,6 +4,8 @@ # Copyright 2017 Google, Inc # +"""Functional tests for checking that patman behaves correctly""" + import os import re import shutil @@ -27,6 +29,7 @@ except ModuleNotFoundError: class TestFunctional(unittest.TestCase): + """Functional tests for checking that patman behaves correctly""" def setUp(self): self.tmpdir = tempfile.mkdtemp(prefix='patman.') self.gitdir = os.path.join(self.tmpdir, 'git') @@ -36,33 +39,69 @@ class TestFunctional(unittest.TestCase): shutil.rmtree(self.tmpdir) @staticmethod - def GetPath(fname): + def _get_path(fname): + """Get the path to a test file + + Args: + fname (str): Filename to obtain + + Returns: + str: Full path to file in the test directory + """ return os.path.join(os.path.dirname(os.path.realpath(sys.argv[0])), 'test', fname) @classmethod - def GetText(self, fname): - return open(self.GetPath(fname), encoding='utf-8').read() + def _get_text(cls, fname): + """Read a file as text + + Args: + fname (str): Filename to read + + Returns: + str: Contents of file + """ + return open(cls._get_path(fname), encoding='utf-8').read() @classmethod - def GetPatchName(self, subject): + def _get_patch_name(cls, subject): + """Get the filename of a patch given its subject + + Args: + subject (str): Patch subject + + Returns: + str: Filename for that patch + """ fname = re.sub('[ :]', '-', subject) return fname.replace('--', '-') - def CreatePatchesForTest(self, series): + def _create_patches_for_test(self, series): + """Create patch files for use by tests + + This copies patch files from the test directory as needed by the series + + Args: + series (Series): Series containing commits to convert + + Returns: + tuple: + str: Cover-letter filename, or None if none + fname_list: list of str, each a patch filename + """ cover_fname = None fname_list = [] for i, commit in enumerate(series.commits): - clean_subject = self.GetPatchName(commit.subject) + clean_subject = self._get_patch_name(commit.subject) src_fname = '%04d-%s.patch' % (i + 1, clean_subject[:52]) fname = os.path.join(self.tmpdir, src_fname) - shutil.copy(self.GetPath(src_fname), fname) + shutil.copy(self._get_path(src_fname), fname) fname_list.append(fname) if series.get('cover'): src_fname = '0000-cover-letter.patch' cover_fname = os.path.join(self.tmpdir, src_fname) fname = os.path.join(self.tmpdir, src_fname) - shutil.copy(self.GetPath(src_fname), fname) + shutil.copy(self._get_path(src_fname), fname) return cover_fname, fname_list @@ -137,7 +176,8 @@ class TestFunctional(unittest.TestCase): stefan = b'Stefan Br\xc3\xbcns '.decode('utf-8') rick = 'Richard III ' mel = b'Lord M\xc3\xablchett '.decode('utf-8') - ed = b'Lond Edmund Blackadd\xc3\xabr Date: Thu, 29 Oct 2020 21:46:16 -0600 Subject: [PATCH 22/44] patman: Allow linking a series with patchwork Add a new Series-links tag to tell patman how to find the series in patchwork. Each item is the series ID optionally preceded by the series version that the link refers to. An empty version indicates this is the latest series. For example: Series-links: 209816 1:203302 Documentation is added in a later patch. Signed-off-by: Simon Glass --- tools/patman/README | 15 +++++++++++++++ tools/patman/func_test.py | 1 + tools/patman/series.py | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/tools/patman/README b/tools/patman/README index 7ede1068ee..6664027ed7 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -187,6 +187,21 @@ Series-name: name patman does not yet use it, but it is convenient to put the branch name here to help you keep track of multiple upstreaming efforts. +Series-links: [id | version:id]... + Set the ID of the series in patchwork. You can set this after you send + out the series and look in patchwork for the resulting series. The + URL you want is the one for the series itself, not any particular patch. + E.g. for http://patchwork.ozlabs.org/project/uboot/list/?series=187331 + the series ID is 187331. This property can have a list of series IDs, + one for each version of the series, e.g. + + Series-links: 1:187331 2:188434 189372 + + Patman always uses the one without a version, since it assumes this is + the latest one. When this tag is provided, patman can compare your local + branch against patchwork to see what new reviews your series has + collected ('patman status'). + Cover-letter: This is the patch set title blah blah diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index b3c3e5796a..ea3c84632c 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -417,6 +417,7 @@ Series for my board This series implements support for my glorious board. END +Series-links: 183237 ''', 'serial.c', '''The code for the serial driver is here''') self.make_commit_with_file('bootm: Make it boot', ''' diff --git a/tools/patman/series.py b/tools/patman/series.py index 9f885c8987..393a44241b 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -16,7 +16,7 @@ from patman import tools # Series-xxx tags that we understand valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name', - 'cover_cc', 'process_log'] + 'cover_cc', 'process_log', 'links'] class Series(dict): """Holds information about a patch series, including all tags. From d06e55a7c66194a0aaedf61ee4762a8e11fc8aac Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:17 -0600 Subject: [PATCH 23/44] patman: Fix indenting in patchstream Update the indenting to keep pylint3 happy. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index ba0a13f632..5fff74918a 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -2,6 +2,8 @@ # Copyright (c) 2011 The Chromium OS Authors. # +"""Handles parsing a stream of commits/emails from 'git log' or other source""" + import datetime import math import os @@ -15,8 +17,8 @@ from patman import gitutil from patman.series import Series # Tags that we detect and remove -re_remove = re.compile('^BUG=|^TEST=|^BRANCH=|^Review URL:' - '|Reviewed-on:|Commit-\w*:') +re_remove = re.compile(r'^BUG=|^TEST=|^BRANCH=|^Review URL:' + r'|Reviewed-on:|Commit-\w*:') # Lines which are allowed after a TEST= line re_allowed_after_test = re.compile('^Signed-off-by:') @@ -46,7 +48,7 @@ re_commit = re.compile('^commit ([0-9a-f]*)$') re_space_before_tab = re.compile('^[+].* \t') # Match indented lines for changes -re_leading_whitespace = re.compile('^\s') +re_leading_whitespace = re.compile(r'^\s') # States we can be in - can we use range() and still have comments? STATE_MSG_HEADER = 0 # Still in the message header @@ -149,7 +151,7 @@ class PatchStream: return int(value) except ValueError as str: raise ValueError("%s: Cannot decode version info '%s'" % - (self.commit.hash, line)) + (self.commit.hash, line)) def FinalizeChange(self): """Finalize a (multi-line) change and add it to the series or commit""" @@ -271,7 +273,7 @@ class PatchStream: # If we are not in a section, it is an unexpected END elif line == 'END': - raise ValueError("'END' wihout section") + raise ValueError("'END' wihout section") # Detect the commit subject elif not is_blank and self.state == STATE_PATCH_SUBJECT: @@ -336,8 +338,9 @@ class PatchStream: value = change_id_match.group(1) if self.is_log: if self.commit.change_id: - raise ValueError("%s: Two Change-Ids: '%s' vs. '%s'" % - (self.commit.hash, self.commit.change_id, value)) + raise ValueError( + "%s: Two Change-Ids: '%s' vs. '%s'" % self.commit.hash, + self.commit.change_id, value) self.commit.change_id = value self.skip_blank = True @@ -353,7 +356,7 @@ class PatchStream: self.change_version = self.ParseVersion(value, line) else: self.warn.append('Line %d: Ignoring Commit-%s' % - (self.linenum, name)) + (self.linenum, name)) # Detect the start of a new commit elif commit_match: @@ -376,7 +379,7 @@ class PatchStream: # Suppress duplicate signoffs elif signoff_match: if (self.is_log or not self.commit or - self.commit.CheckDuplicateSignoff(signoff_match.group(1))): + self.commit.CheckDuplicateSignoff(signoff_match.group(1))): out = [line] # Well that means this is an ordinary line @@ -385,7 +388,7 @@ class PatchStream: m = re_space_before_tab.match(line) if m: self.warn.append('Line %d/%d has space before tab' % - (self.linenum, m.start())) + (self.linenum, m.start())) # OK, we have a valid non-blank line out = [line] @@ -418,7 +421,7 @@ class PatchStream: self.CloseCommit() if self.lines_after_test: self.warn.append('Found %d lines after TEST=' % - self.lines_after_test) + self.lines_after_test) def WriteMessageId(self, outfd): """Write the Message-Id into the output. @@ -494,7 +497,7 @@ class PatchStream: else: if self.blank_count and (line == '-- ' or match): self.warn.append("Found possible blank line(s) at " - "end of file '%s'" % last_fname) + "end of file '%s'" % last_fname) outfd.write('+\n' * self.blank_count) outfd.write(line + '\n') self.blank_count = 0 @@ -502,7 +505,7 @@ class PatchStream: def GetMetaDataForList(commit_range, git_dir=None, count=None, - series = None, allow_overwrite=False): + series=None, allow_overwrite=False): """Reads out patch series metadata from the commits This does a 'git log' on the relevant commits and pulls out the tags we From 5769904082aa976e7d1676adc673977c9f8c7717 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:18 -0600 Subject: [PATCH 24/44] patman: Fix constant style in patchstream These constants should use upper case. Update them to keep pylint3 happy. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 44 ++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 5fff74918a..86b03a7a30 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -17,38 +17,38 @@ from patman import gitutil from patman.series import Series # Tags that we detect and remove -re_remove = re.compile(r'^BUG=|^TEST=|^BRANCH=|^Review URL:' +RE_REMOVE = re.compile(r'^BUG=|^TEST=|^BRANCH=|^Review URL:' r'|Reviewed-on:|Commit-\w*:') # Lines which are allowed after a TEST= line -re_allowed_after_test = re.compile('^Signed-off-by:') +RE_ALLOWED_AFTER_TEST = re.compile('^Signed-off-by:') # Signoffs -re_signoff = re.compile('^Signed-off-by: *(.*)') +RE_SIGNOFF = re.compile('^Signed-off-by: *(.*)') # Cover letter tag -re_cover = re.compile('^Cover-([a-z-]*): *(.*)') +RE_COVER = re.compile('^Cover-([a-z-]*): *(.*)') # Patch series tag -re_series_tag = re.compile('^Series-([a-z-]*): *(.*)') +RE_SERIES_TAG = re.compile('^Series-([a-z-]*): *(.*)') # Change-Id will be used to generate the Message-Id and then be stripped -re_change_id = re.compile('^Change-Id: *(.*)') +RE_CHANGE_ID = re.compile('^Change-Id: *(.*)') # Commit series tag -re_commit_tag = re.compile('^Commit-([a-z-]*): *(.*)') +RE_COMMIT_TAG = re.compile('^Commit-([a-z-]*): *(.*)') # Commit tags that we want to collect and keep -re_tag = re.compile('^(Tested-by|Acked-by|Reviewed-by|Patch-cc|Fixes): (.*)') +RE_TAG = re.compile('^(Tested-by|Acked-by|Reviewed-by|Patch-cc|Fixes): (.*)') # The start of a new commit in the git log -re_commit = re.compile('^commit ([0-9a-f]*)$') +RE_COMMIT = re.compile('^commit ([0-9a-f]*)$') # We detect these since checkpatch doesn't always do it -re_space_before_tab = re.compile('^[+].* \t') +RE_SPACE_BEFORE_TAB = re.compile('^[+].* \t') # Match indented lines for changes -re_leading_whitespace = re.compile(r'^\s') +RE_LEADING_WHITESPACE = re.compile(r'^\s') # States we can be in - can we use range() and still have comments? STATE_MSG_HEADER = 0 # Still in the message header @@ -195,22 +195,22 @@ class PatchStream: out = [] line = line.rstrip('\n') - commit_match = re_commit.match(line) if self.is_log else None + commit_match = RE_COMMIT.match(line) if self.is_log else None if self.is_log: if line[:4] == ' ': line = line[4:] # Handle state transition and skipping blank lines - series_tag_match = re_series_tag.match(line) - change_id_match = re_change_id.match(line) - commit_tag_match = re_commit_tag.match(line) - cover_match = re_cover.match(line) - signoff_match = re_signoff.match(line) - leading_whitespace_match = re_leading_whitespace.match(line) + series_tag_match = RE_SERIES_TAG.match(line) + change_id_match = RE_CHANGE_ID.match(line) + commit_tag_match = RE_COMMIT_TAG.match(line) + cover_match = RE_COVER.match(line) + signoff_match = RE_SIGNOFF.match(line) + leading_whitespace_match = RE_LEADING_WHITESPACE.match(line) tag_match = None if self.state == STATE_PATCH_HEADER: - tag_match = re_tag.match(line) + tag_match = RE_TAG.match(line) is_blank = not line.strip() if is_blank: if (self.state == STATE_MSG_HEADER @@ -280,7 +280,7 @@ class PatchStream: self.commit.subject = line # Detect the tags we want to remove, and skip blank lines - elif re_remove.match(line) and not commit_tag_match: + elif RE_REMOVE.match(line) and not commit_tag_match: self.skip_blank = True # TEST= should be the last thing in the commit, so remove @@ -385,7 +385,7 @@ class PatchStream: # Well that means this is an ordinary line else: # Look for space before tab - m = re_space_before_tab.match(line) + m = RE_SPACE_BEFORE_TAB.match(line) if m: self.warn.append('Line %d/%d has space before tab' % (self.linenum, m.start())) @@ -410,7 +410,7 @@ class PatchStream: out += self.commit.notes out += [''] + log elif self.found_test: - if not re_allowed_after_test.match(line): + if not RE_ALLOWED_AFTER_TEST.match(line): self.lines_after_test += 1 return out From d93720e13800423d3d6c9f8318d5a75ce7362639 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:19 -0600 Subject: [PATCH 25/44] patman: Rename functions in patchstream Rename these functions to lower case as per PEP8. Signed-off-by: Simon Glass --- tools/buildman/control.py | 6 +-- tools/patman/control.py | 6 +-- tools/patman/func_test.py | 6 +-- tools/patman/patchstream.py | 88 ++++++++++++++++----------------- tools/patman/test_checkpatch.py | 6 +-- 5 files changed, 56 insertions(+), 56 deletions(-) diff --git a/tools/buildman/control.py b/tools/buildman/control.py index b81ecf6a53..fe874b8165 100644 --- a/tools/buildman/control.py +++ b/tools/buildman/control.py @@ -276,14 +276,14 @@ def DoBuildman(options, args, toolchains=None, make_func=None, boards=None, options.branch) upstream_commit = gitutil.GetUpstream(options.git_dir, options.branch) - series = patchstream.GetMetaDataForList(upstream_commit, + series = patchstream.get_metadata_for_list(upstream_commit, options.git_dir, 1, series=None, allow_overwrite=True) - series = patchstream.GetMetaDataForList(range_expr, + series = patchstream.get_metadata_for_list(range_expr, options.git_dir, None, series, allow_overwrite=True) else: # Honour the count - series = patchstream.GetMetaDataForList(options.branch, + series = patchstream.get_metadata_for_list(options.branch, options.git_dir, count, series=None, allow_overwrite=True) else: series = None diff --git a/tools/patman/control.py b/tools/patman/control.py index aea9df8c8d..6555a4018a 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -54,14 +54,14 @@ def prepare_patches(col, branch, count, start, end, ignore_binary): # Read the metadata from the commits to_do = count - end - series = patchstream.GetMetaData(branch, start, to_do) + series = patchstream.get_metadata(branch, start, to_do) cover_fname, patch_files = gitutil.CreatePatches( branch, start, to_do, ignore_binary, series) # Fix up the patch files to our liking, and insert the cover letter - patchstream.FixPatches(series, patch_files) + patchstream.fix_patches(series, patch_files) if cover_fname and series.get('cover'): - patchstream.InsertCoverLetter(cover_fname, series, to_do) + patchstream.insert_cover_letter(cover_fname, series, to_do) return series, cover_fname, patch_files def check_patches(series, patch_files, run_checkpatch, verbose): diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index ea3c84632c..bdeccafda0 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -191,12 +191,12 @@ class TestFunctional(unittest.TestCase): } text = self._get_text('test01.txt') - series = patchstream.GetMetaDataForTest(text) + series = patchstream.get_metadata_for_test(text) cover_fname, args = self._create_patches_for_test(series) with capture_sys_output() as out: - patchstream.FixPatches(series, args) + patchstream.fix_patches(series, args) if cover_fname and series.get('cover'): - patchstream.InsertCoverLetter(cover_fname, series, count) + patchstream.insert_cover_letter(cover_fname, series, count) series.DoChecks() cc_file = series.MakeCcFile(process_tags, cover_fname, not ignore_bad_tags, add_maintainers, diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 86b03a7a30..5487799bbe 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -83,7 +83,7 @@ class PatchStream: self.signoff = [] # Contents of signoff line self.commit = None # Current commit - def AddToSeries(self, line, name, value): + def _add_to_series(self, line, name, value): """Add a new Series-xxx tag. When a Series-xxx tag is detected, we come here to record it, if we @@ -100,7 +100,7 @@ class PatchStream: if self.is_log: self.series.AddTag(self.commit, line, name, value) - def AddToCommit(self, line, name, value): + def _add_to_commit(self, line, name, value): """Add a new Commit-xxx tag. When a Commit-xxx tag is detected, we come here to record it. @@ -114,7 +114,7 @@ class PatchStream: self.in_section = 'commit-' + name self.skip_blank = False - def AddCommitRtag(self, rtag_type, who): + def _add_commit_rtag(self, rtag_type, who): """Add a response tag to the current commit Args: @@ -123,7 +123,7 @@ class PatchStream: """ self.commit.AddRtag(rtag_type, who) - def CloseCommit(self): + def _close_commit(self): """Save the current commit into our commit list, and reset our state""" if self.commit and self.is_log: self.series.AddCommit(self.commit) @@ -137,7 +137,7 @@ class PatchStream: self.skip_blank = True self.section = [] - def ParseVersion(self, value, line): + def _parse_version(self, value, line): """Parse a version from a *-changes tag Args: @@ -153,8 +153,8 @@ class PatchStream: raise ValueError("%s: Cannot decode version info '%s'" % (self.commit.hash, line)) - def FinalizeChange(self): - """Finalize a (multi-line) change and add it to the series or commit""" + def _finalise_change(self): + """_finalise a (multi-line) change and add it to the series or commit""" if not self.change_lines: return change = '\n'.join(self.change_lines) @@ -167,7 +167,7 @@ class PatchStream: self.commit.AddChange(self.change_version, change) self.change_lines = [] - def ProcessLine(self, line): + def process_line(self, line): """Process a single line of a patch file or commit log This process a line and returns a list of lines to output. The list @@ -248,7 +248,7 @@ class PatchStream: # is missing, fix it up. if self.in_change: self.warn.append("Missing 'blank line' in section '%s-changes'" % self.in_change) - self.FinalizeChange() + self._finalise_change() self.in_change = None self.change_version = 0 @@ -298,26 +298,26 @@ class PatchStream: self.in_section = 'cover' self.skip_blank = False elif name == 'letter-cc': - self.AddToSeries(line, 'cover-cc', value) + self._add_to_series(line, 'cover-cc', value) elif name == 'changes': self.in_change = 'Cover' - self.change_version = self.ParseVersion(value, line) + self.change_version = self._parse_version(value, line) # If we are in a change list, key collected lines until a blank one elif self.in_change: if is_blank: # Blank line ends this change list - self.FinalizeChange() + self._finalise_change() self.in_change = None self.change_version = 0 elif line == '---': - self.FinalizeChange() + self._finalise_change() self.in_change = None self.change_version = 0 - out = self.ProcessLine(line) + out = self.process_line(line) elif self.is_log: if not leading_whitespace_match: - self.FinalizeChange() + self._finalise_change() self.change_lines.append(line) self.skip_blank = False @@ -328,9 +328,9 @@ class PatchStream: if name == 'changes': # value is the version number: e.g. 1, or 2 self.in_change = 'Series' - self.change_version = self.ParseVersion(value, line) + self.change_version = self._parse_version(value, line) else: - self.AddToSeries(line, name, value) + self._add_to_series(line, name, value) self.skip_blank = True # Detect Change-Id tags @@ -349,24 +349,24 @@ class PatchStream: name = commit_tag_match.group(1) value = commit_tag_match.group(2) if name == 'notes': - self.AddToCommit(line, name, value) + self._add_to_commit(line, name, value) self.skip_blank = True elif name == 'changes': self.in_change = 'Commit' - self.change_version = self.ParseVersion(value, line) + self.change_version = self._parse_version(value, line) else: self.warn.append('Line %d: Ignoring Commit-%s' % (self.linenum, name)) # Detect the start of a new commit elif commit_match: - self.CloseCommit() + self._close_commit() self.commit = commit.Commit(commit_match.group(1)) # Detect tags in the commit message elif tag_match: rtag_type, who = tag_match.groups() - self.AddCommitRtag(rtag_type, who) + self._add_commit_rtag(rtag_type, who) # Remove Tested-by self, since few will take much notice if (rtag_type == 'Tested-by' and who.find(os.getenv('USER') + '@') != -1): @@ -415,15 +415,15 @@ class PatchStream: return out - def Finalize(self): + def finalise(self): """Close out processing of this patch stream""" - self.FinalizeChange() - self.CloseCommit() + self._finalise_change() + self._close_commit() if self.lines_after_test: self.warn.append('Found %d lines after TEST=' % self.lines_after_test) - def WriteMessageId(self, outfd): + def _write_message_id(self, outfd): """Write the Message-Id into the output. This is based on the Change-Id in the original patch, the version, @@ -464,7 +464,7 @@ class PatchStream: # Join parts together with "." and write it out. outfd.write('Message-Id: <%s@changeid>\n' % '.'.join(parts)) - def ProcessStream(self, infd, outfd): + def process_stream(self, infd, outfd): """Copy a stream from infd to outfd, filtering out unwanting things. This is used to process patch files one at a time. @@ -478,13 +478,13 @@ class PatchStream: last_fname = None re_fname = re.compile('diff --git a/(.*) b/.*') - self.WriteMessageId(outfd) + self._write_message_id(outfd) while True: line = infd.readline() if not line: break - out = self.ProcessLine(line) + out = self.process_line(line) # Try to detect blank lines at EOF for line in out: @@ -501,11 +501,11 @@ class PatchStream: outfd.write('+\n' * self.blank_count) outfd.write(line + '\n') self.blank_count = 0 - self.Finalize() + self.finalise() -def GetMetaDataForList(commit_range, git_dir=None, count=None, - series=None, allow_overwrite=False): +def get_metadata_for_list(commit_range, git_dir=None, count=None, + series=None, allow_overwrite=False): """Reads out patch series metadata from the commits This does a 'git log' on the relevant commits and pulls out the tags we @@ -529,11 +529,11 @@ def GetMetaDataForList(commit_range, git_dir=None, count=None, stdout = command.RunPipe([params], capture=True).stdout ps = PatchStream(series, is_log=True) for line in stdout.splitlines(): - ps.ProcessLine(line) - ps.Finalize() + ps.process_line(line) + ps.finalise() return series -def GetMetaData(branch, start, count): +def get_metadata(branch, start, count): """Reads out patch series metadata from the commits This does a 'git log' on the relevant commits and pulls out the tags we @@ -544,10 +544,10 @@ def GetMetaData(branch, start, count): start: Commit to start from: 0=branch HEAD, 1=next one, etc. count: Number of commits to list """ - return GetMetaDataForList('%s~%d' % (branch if branch else 'HEAD', start), - None, count) + return get_metadata_for_list( + '%s~%d' % (branch if branch else 'HEAD', start), None, count) -def GetMetaDataForTest(text): +def get_metadata_for_test(text): """Process metadata from a file containing a git log. Used for tests Args: @@ -556,11 +556,11 @@ def GetMetaDataForTest(text): series = Series() ps = PatchStream(series, is_log=True) for line in text.splitlines(): - ps.ProcessLine(line) - ps.Finalize() + ps.process_line(line) + ps.finalise() return series -def FixPatch(backup_dir, fname, series, commit): +def fix_patch(backup_dir, fname, series, commit): """Fix up a patch file, by adding/removing as required. We remove our tags from the patch file, insert changes lists, etc. @@ -580,7 +580,7 @@ def FixPatch(backup_dir, fname, series, commit): infd = open(fname, 'r', encoding='utf-8') ps = PatchStream(series) ps.commit = commit - ps.ProcessStream(infd, outfd) + ps.process_stream(infd, outfd) infd.close() outfd.close() @@ -590,7 +590,7 @@ def FixPatch(backup_dir, fname, series, commit): shutil.move(tmpname, fname) return ps.warn -def FixPatches(series, fnames): +def fix_patches(series, fnames): """Fix up a list of patches identified by filenames The patch files are processed in place, and overwritten. @@ -606,7 +606,7 @@ def FixPatches(series, fnames): commit = series.commits[count] commit.patch = fname commit.count = count - result = FixPatch(backup_dir, fname, series, commit) + result = fix_patch(backup_dir, fname, series, commit) if result: print('%d warnings for %s:' % (len(result), fname)) for warn in result: @@ -615,7 +615,7 @@ def FixPatches(series, fnames): count += 1 print('Cleaned %d patches' % count) -def InsertCoverLetter(fname, series, count): +def insert_cover_letter(fname, series, count): """Inserts a cover letter with the required info into patch 0 Args: diff --git a/tools/patman/test_checkpatch.py b/tools/patman/test_checkpatch.py index f71c70fb13..1f7c38c4e9 100644 --- a/tools/patman/test_checkpatch.py +++ b/tools/patman/test_checkpatch.py @@ -148,15 +148,15 @@ Signed-off-by: Simon Glass expfd.write(expected) expfd.close() - # Normally by the time we call FixPatch we've already collected + # Normally by the time we call fix_patch we've already collected # metadata. Here, we haven't, but at least fake up something. - # Set the "count" to -1 which tells FixPatch to use a bogus/fixed + # Set the "count" to -1 which tells fix_patch to use a bogus/fixed # time for generating the Message-Id. com = commit.Commit('') com.change_id = 'I80fe1d0c0b7dd10aa58ce5bb1d9290b6664d5413' com.count = -1 - patchstream.FixPatch(None, inname, series.Series(), com) + patchstream.fix_patch(None, inname, series.Series(), com) rc = os.system('diff -u %s %s' % (inname, expname)) self.assertEqual(rc, 0) From dd147eda10d24a04ed3378bfc8679ac25160f96c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:20 -0600 Subject: [PATCH 26/44] patman: Rename variables in patchstream Some variables are too short or shadow other variables or types. Fix these to keep pylint3 happy. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 52 ++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 5487799bbe..3a057fbd6c 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -149,7 +149,7 @@ class PatchStream: """ try: return int(value) - except ValueError as str: + except ValueError: raise ValueError("%s: Cannot decode version info '%s'" % (self.commit.hash, line)) @@ -385,10 +385,10 @@ class PatchStream: # Well that means this is an ordinary line else: # Look for space before tab - m = RE_SPACE_BEFORE_TAB.match(line) - if m: + mat = RE_SPACE_BEFORE_TAB.match(line) + if mat: self.warn.append('Line %d/%d has space before tab' % - (self.linenum, m.start())) + (self.linenum, mat.start())) # OK, we have a valid non-blank line out = [line] @@ -527,10 +527,10 @@ def get_metadata_for_list(commit_range, git_dir=None, count=None, params = gitutil.LogCmd(commit_range, reverse=True, count=count, git_dir=git_dir) stdout = command.RunPipe([params], capture=True).stdout - ps = PatchStream(series, is_log=True) + pst = PatchStream(series, is_log=True) for line in stdout.splitlines(): - ps.process_line(line) - ps.finalise() + pst.process_line(line) + pst.finalise() return series def get_metadata(branch, start, count): @@ -554,13 +554,13 @@ def get_metadata_for_test(text): text: """ series = Series() - ps = PatchStream(series, is_log=True) + pst = PatchStream(series, is_log=True) for line in text.splitlines(): - ps.process_line(line) - ps.finalise() + pst.process_line(line) + pst.finalise() return series -def fix_patch(backup_dir, fname, series, commit): +def fix_patch(backup_dir, fname, series, cmt): """Fix up a patch file, by adding/removing as required. We remove our tags from the patch file, insert changes lists, etc. @@ -571,16 +571,16 @@ def fix_patch(backup_dir, fname, series, commit): Args: fname: Filename to patch file to process series: Series information about this patch set - commit: Commit object for this patch file + cmt: Commit object for this patch file Return: A list of errors, or [] if all ok. """ handle, tmpname = tempfile.mkstemp() outfd = os.fdopen(handle, 'w', encoding='utf-8') infd = open(fname, 'r', encoding='utf-8') - ps = PatchStream(series) - ps.commit = commit - ps.process_stream(infd, outfd) + pst = PatchStream(series) + pst.commit = cmt + pst.process_stream(infd, outfd) infd.close() outfd.close() @@ -588,7 +588,7 @@ def fix_patch(backup_dir, fname, series, commit): if backup_dir: shutil.copy(fname, os.path.join(backup_dir, os.path.basename(fname))) shutil.move(tmpname, fname) - return ps.warn + return pst.warn def fix_patches(series, fnames): """Fix up a list of patches identified by filenames @@ -603,10 +603,10 @@ def fix_patches(series, fnames): backup_dir = None #tempfile.mkdtemp('clean-patch') count = 0 for fname in fnames: - commit = series.commits[count] - commit.patch = fname - commit.count = count - result = fix_patch(backup_dir, fname, series, commit) + cmt = series.commits[count] + cmt.patch = fname + cmt.count = count + result = fix_patch(backup_dir, fname, series, cmt) if result: print('%d warnings for %s:' % (len(result), fname)) for warn in result: @@ -623,11 +623,11 @@ def insert_cover_letter(fname, series, count): series: Series object count: Number of patches in the series """ - fd = open(fname, 'r') - lines = fd.readlines() - fd.close() + fil = open(fname, 'r') + lines = fil.readlines() + fil.close() - fd = open(fname, 'w') + fil = open(fname, 'w') text = series.cover prefix = series.GetPatchPrefix() for line in lines: @@ -647,5 +647,5 @@ def insert_cover_letter(fname, series, count): # Now the change list out = series.MakeChangeLog(None) line += '\n' + '\n'.join(out) - fd.write(line) - fd.close() + fil.write(line) + fil.close() From e3a816b9f4ae760fc76ac04b1c730ef5a9f1c1cc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:21 -0600 Subject: [PATCH 27/44] patman: Drop unused args in patchstream Drop a few arguments that are not used in functions. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 3a057fbd6c..c5402dd896 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -64,7 +64,7 @@ class PatchStream: unwanted tags or inject additional ones. These correspond to the two phases of processing. """ - def __init__(self, series, name=None, is_log=False): + def __init__(self, series, is_log=False): self.skip_blank = False # True to skip a single blank line self.found_test = False # Found a TEST= line self.lines_after_test = 0 # Number of lines found after TEST= @@ -100,15 +100,13 @@ class PatchStream: if self.is_log: self.series.AddTag(self.commit, line, name, value) - def _add_to_commit(self, line, name, value): + def _add_to_commit(self, name): """Add a new Commit-xxx tag. When a Commit-xxx tag is detected, we come here to record it. Args: - line: Source line containing tag (useful for debug/error messages) name: Tag name (part after 'Commit-') - value: Tag value (part after 'Commit-xxx: ') """ if name == 'notes': self.in_section = 'commit-' + name @@ -349,7 +347,7 @@ class PatchStream: name = commit_tag_match.group(1) value = commit_tag_match.group(2) if name == 'notes': - self._add_to_commit(line, name, value) + self._add_to_commit(name) self.skip_blank = True elif name == 'changes': self.in_change = 'Commit' From 1cb1c0fc8d6b797901f68afe5e9dd18c20401c5e Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:22 -0600 Subject: [PATCH 28/44] patman: Fix up argument/return docs in patchstream Add missing documentation and type information. Fix up some missing docs on exceptions also. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 82 ++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index c5402dd896..b2cb2debee 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -90,9 +90,10 @@ class PatchStream: are scanning a 'git log'. Args: - line: Source line containing tag (useful for debug/error messages) - name: Tag name (part after 'Series-') - value: Tag value (part after 'Series-xxx: ') + line (str): Source line containing tag (useful for debug/error + messages) + name (str): Tag name (part after 'Series-') + value (str): Tag value (part after 'Series-xxx: ') """ if name == 'notes': self.in_section = name @@ -106,7 +107,7 @@ class PatchStream: When a Commit-xxx tag is detected, we come here to record it. Args: - name: Tag name (part after 'Commit-') + name (str): Tag name (part after 'Commit-') """ if name == 'notes': self.in_section = 'commit-' + name @@ -116,8 +117,9 @@ class PatchStream: """Add a response tag to the current commit Args: - key: rtag type (e.g. 'Reviewed-by') - who: Person who gave that rtag, e.g. 'Fred Bloggs ' + rtag_type (str): rtag type (e.g. 'Reviewed-by') + who (str): Person who gave that rtag, e.g. + 'Fred Bloggs ' """ self.commit.AddRtag(rtag_type, who) @@ -139,11 +141,14 @@ class PatchStream: """Parse a version from a *-changes tag Args: - value: Tag value (part after 'xxx-changes: ' - line: Source line containing tag + value (str): Tag value (part after 'xxx-changes: ' + line (str): Source line containing tag Returns: - The version as an integer + int: The version as an integer + + Raises: + ValueError: the value cannot be converted """ try: return int(value) @@ -184,10 +189,14 @@ class PatchStream: don't want, and add things we think are required. Args: - line: text line to process + line (str): text line to process Returns: - list of output lines, or [] if nothing should be output + list: list of output lines, or [] if nothing should be output + + Raises: + ValueError: a fatal error occurred while parsing, e.g. an END + without a starting tag, or two commits with two change IDs """ # Initially we have no output. Prepare the input line string out = [] @@ -428,7 +437,7 @@ class PatchStream: and the prefix. Args: - outfd: Output stream file object + outfd (io.IOBase): Output stream file object """ if not self.commit.change_id: return @@ -468,8 +477,8 @@ class PatchStream: This is used to process patch files one at a time. Args: - infd: Input stream file object - outfd: Output stream file object + infd (io.IOBase): Input stream file object + outfd (io.IOBase): Output stream file object """ # Extract the filename from each diff, for nice warnings fname = None @@ -510,14 +519,15 @@ def get_metadata_for_list(commit_range, git_dir=None, count=None, are interested in. Args: - commit_range: Range of commits to count (e.g. 'HEAD..base') - git_dir: Path to git repositiory (None to use default) - count: Number of commits to list, or None for no limit - series: Series object to add information into. By default a new series + commit_range (str): Range of commits to count (e.g. 'HEAD..base') + git_dir (str): Path to git repositiory (None to use default) + count (int): Number of commits to list, or None for no limit + series (Series): Object to add information into. By default a new series is started. - allow_overwrite: Allow tags to overwrite an existing tag + allow_overwrite (bool): Allow tags to overwrite an existing tag + Returns: - A Series object containing information about the commits. + Series: Object containing information about the commits. """ if not series: series = Series() @@ -538,9 +548,12 @@ def get_metadata(branch, start, count): are interested in. Args: - branch: Branch to use (None for current branch) - start: Commit to start from: 0=branch HEAD, 1=next one, etc. - count: Number of commits to list + branch (str): Branch to use (None for current branch) + start (int): Commit to start from: 0=branch HEAD, 1=next one, etc. + count (int): Number of commits to list + + Returns: + Series: Object containing information about the commits. """ return get_metadata_for_list( '%s~%d' % (branch if branch else 'HEAD', start), None, count) @@ -550,6 +563,9 @@ def get_metadata_for_test(text): Args: text: + + Returns: + Series: Object containing information about the commits. """ series = Series() pst = PatchStream(series, is_log=True) @@ -567,11 +583,13 @@ def fix_patch(backup_dir, fname, series, cmt): A backup file is put into backup_dir (if not None). Args: - fname: Filename to patch file to process - series: Series information about this patch set - cmt: Commit object for this patch file + backup_dir (str): Path to directory to use to backup the file + fname (str): Filename to patch file to process + series (Series): Series information about this patch set + cmt (Commit): Commit object for this patch file + Return: - A list of errors, or [] if all ok. + list: A list of errors, each str, or [] if all ok. """ handle, tmpname = tempfile.mkstemp() outfd = os.fdopen(handle, 'w', encoding='utf-8') @@ -594,8 +612,8 @@ def fix_patches(series, fnames): The patch files are processed in place, and overwritten. Args: - series: The series object - fnames: List of patch files to process + series (Series): The Series object + fnames (:type: list of str): List of patch files to process """ # Current workflow creates patches, so we shouldn't need a backup backup_dir = None #tempfile.mkdtemp('clean-patch') @@ -617,9 +635,9 @@ def insert_cover_letter(fname, series, count): """Inserts a cover letter with the required info into patch 0 Args: - fname: Input / output filename of the cover letter file - series: Series object - count: Number of patches in the series + fname (str): Input / output filename of the cover letter file + series (Series): Series object + count (int): Number of patches in the series """ fil = open(fname, 'r') lines = fil.readlines() From b5cc39905343312d1b7064ede72561394049be21 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:23 -0600 Subject: [PATCH 29/44] patman: Move warning collection to a function Add a new function in PatchStream to collect the warnings generated while parsing the stream. This will allow us to adjust the logic, such as dealing with per-commit warnings. Two of the warnings are in fact internal errors, so change them to raise and exception. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index b2cb2debee..9f283470bc 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -83,6 +83,14 @@ class PatchStream: self.signoff = [] # Contents of signoff line self.commit = None # Current commit + def _add_warn(self, warn): + """Add a new warning to report to the user + + Args: + warn (str): Warning to report + """ + self.warn.append(warn) + def _add_to_series(self, line, name, value): """Add a new Series-xxx tag. @@ -237,7 +245,7 @@ class PatchStream: # but we are already in a section, this means 'END' is missing # for that section, fix it up. if self.in_section: - self.warn.append("Missing 'END' in section '%s'" % self.in_section) + self._add_warn("Missing 'END' in section '%s'" % self.in_section) if self.in_section == 'cover': self.series.cover = self.section elif self.in_section == 'notes': @@ -247,14 +255,16 @@ class PatchStream: if self.is_log: self.commit.notes += self.section else: - self.warn.append("Unknown section '%s'" % self.in_section) + # This should not happen + raise ValueError("Unknown section '%s'" % self.in_section) self.in_section = None self.skip_blank = True self.section = [] # but we are already in a change list, that means a blank line # is missing, fix it up. if self.in_change: - self.warn.append("Missing 'blank line' in section '%s-changes'" % self.in_change) + self._add_warn("Missing 'blank line' in section '%s-changes'" % + self.in_change) self._finalise_change() self.in_change = None self.change_version = 0 @@ -271,7 +281,8 @@ class PatchStream: if self.is_log: self.commit.notes += self.section else: - self.warn.append("Unknown section '%s'" % self.in_section) + # This should not happen + raise ValueError("Unknown section '%s'" % self.in_section) self.in_section = None self.skip_blank = True self.section = [] @@ -362,8 +373,8 @@ class PatchStream: self.in_change = 'Commit' self.change_version = self._parse_version(value, line) else: - self.warn.append('Line %d: Ignoring Commit-%s' % - (self.linenum, name)) + self._add_warn('Line %d: Ignoring Commit-%s' % + (self.linenum, name)) # Detect the start of a new commit elif commit_match: @@ -377,7 +388,7 @@ class PatchStream: # Remove Tested-by self, since few will take much notice if (rtag_type == 'Tested-by' and who.find(os.getenv('USER') + '@') != -1): - self.warn.append("Ignoring %s" % line) + self._add_warn("Ignoring %s" % line) elif rtag_type == 'Patch-cc': self.commit.AddCc(who.split(',')) else: @@ -394,8 +405,8 @@ class PatchStream: # Look for space before tab mat = RE_SPACE_BEFORE_TAB.match(line) if mat: - self.warn.append('Line %d/%d has space before tab' % - (self.linenum, mat.start())) + self._add_warn('Line %d/%d has space before tab' % + (self.linenum, mat.start())) # OK, we have a valid non-blank line out = [line] @@ -427,8 +438,7 @@ class PatchStream: self._finalise_change() self._close_commit() if self.lines_after_test: - self.warn.append('Found %d lines after TEST=' % - self.lines_after_test) + self._add_warn('Found %d lines after TEST=' % self.lines_after_test) def _write_message_id(self, outfd): """Write the Message-Id into the output. @@ -503,8 +513,8 @@ class PatchStream: self.blank_count += 1 else: if self.blank_count and (line == '-- ' or match): - self.warn.append("Found possible blank line(s) at " - "end of file '%s'" % last_fname) + self._add_warn("Found possible blank line(s) at end of file '%s'" % + last_fname) outfd.write('+\n' * self.blank_count) outfd.write(line + '\n') self.blank_count = 0 From 313ef5f897f15bc7ec2ca8054f4e4871a2f6be93 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:24 -0600 Subject: [PATCH 30/44] patman: Attach warnings to individual patches At present warnings are produced across the whole set of patches when parsing them. It is more useful to associate each warning with the patch (or commit) that generated it. Attach warnings to the Commit object and move them out of PatchStream. Also avoid generating duplicate warnings for the same commit. Signed-off-by: Simon Glass --- tools/patman/commit.py | 2 ++ tools/patman/patchstream.py | 15 +++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/tools/patman/commit.py b/tools/patman/commit.py index 8d583c4ed3..e49bf87dfc 100644 --- a/tools/patman/commit.py +++ b/tools/patman/commit.py @@ -27,6 +27,7 @@ class Commit: rtags: Response tags (e.g. Reviewed-by) collected by the commit, dict: key: rtag type (e.g. 'Reviewed-by') value: Set of people who gave that rtag, each a name/email string + warn: List of warnings for this commit, each a str """ def __init__(self, hash): self.hash = hash @@ -38,6 +39,7 @@ class Commit: self.notes = [] self.change_id = None self.rtags = collections.defaultdict(set) + self.warn = [] def AddChange(self, version, info): """Add a new change line to the change list for a version. diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 9f283470bc..880d7ddc7f 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -68,7 +68,6 @@ class PatchStream: self.skip_blank = False # True to skip a single blank line self.found_test = False # Found a TEST= line self.lines_after_test = 0 # Number of lines found after TEST= - self.warn = [] # List of warnings we have collected self.linenum = 1 # Output line number we are up to self.in_section = None # Name of start...END section we are in self.notes = [] # Series notes @@ -84,12 +83,20 @@ class PatchStream: self.commit = None # Current commit def _add_warn(self, warn): - """Add a new warning to report to the user + """Add a new warning to report to the user about the current commit + + The new warning is added to the current commit if not already present. Args: warn (str): Warning to report + + Raises: + ValueError: Warning is generated with no commit associated """ - self.warn.append(warn) + if not self.commit: + raise ValueError('Warning outside commit: %s' % warn) + if warn not in self.commit.warn: + self.commit.warn.append(warn) def _add_to_series(self, line, name, value): """Add a new Series-xxx tag. @@ -614,7 +621,7 @@ def fix_patch(backup_dir, fname, series, cmt): if backup_dir: shutil.copy(fname, os.path.join(backup_dir, os.path.basename(fname))) shutil.move(tmpname, fname) - return pst.warn + return cmt.warn def fix_patches(series, fnames): """Fix up a list of patches identified by filenames From dffa42c3ef98a8f9bc26a221cdfc3fcfb9f0fa40 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:25 -0600 Subject: [PATCH 31/44] patman: Convert 'Series-xxx' tag errors into warnings If the Series-xxx tag is not recognised patman currently reports a fatal error. This is inconvenient if a new feature is later added to patman that an earlier version does not support. Report a warning instead, to allow the user to take action if needed, but still allow operation to proceed. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 4 +++- tools/patman/series.py | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 880d7ddc7f..24040d43d6 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -114,7 +114,9 @@ class PatchStream: self.in_section = name self.skip_blank = False if self.is_log: - self.series.AddTag(self.commit, line, name, value) + warn = self.series.AddTag(self.commit, line, name, value) + if warn: + self.commit.warn.append(warn) def _add_to_commit(self, name): """Add a new Commit-xxx tag. diff --git a/tools/patman/series.py b/tools/patman/series.py index 393a44241b..4457719f2e 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -59,6 +59,9 @@ class Series(dict): line: Source line containing tag (useful for debug/error messages) name: Tag name (part after 'Series-') value: Tag value (part after 'Series-xxx: ') + + Returns: + String warning if something went wrong, else None """ # If we already have it, then add to our list name = name.replace('-', '_') @@ -78,9 +81,10 @@ class Series(dict): else: self[name] = value else: - raise ValueError("In %s: line '%s': Unknown 'Series-%s': valid " + return ("In %s: line '%s': Unknown 'Series-%s': valid " "options are %s" % (commit.hash, line, name, ', '.join(valid_series))) + return None def AddCommit(self, commit): """Add a commit into our list of commits From 47f62952cce810c6e02eb216ec32ce69713534a7 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:26 -0600 Subject: [PATCH 32/44] patman: Drop unused signoff member This is not used. Drop it. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 1 + tools/patman/patchstream.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index bdeccafda0..2290ba95e9 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -16,6 +16,7 @@ import unittest from patman import control from patman import gitutil from patman import patchstream +from patman.patchstream import PatchStream from patman import settings from patman import terminal from patman import tools diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 24040d43d6..cf591b2757 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -79,7 +79,6 @@ class PatchStream: self.change_lines = [] # Lines of the current change self.blank_count = 0 # Number of blank lines stored up self.state = STATE_MSG_HEADER # What state are we in? - self.signoff = [] # Contents of signoff line self.commit = None # Current commit def _add_warn(self, warn): From 7457051e41be1058494bcb96f8ebd05176a3e6cc Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:27 -0600 Subject: [PATCH 33/44] patman: Add a test for PatchStream tags The current functional tests run most of patman. Add a smaller test that just checks tag handling with the PatchStream class. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 26 +++++++++++++++++++++----- tools/patman/patchstream.py | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 2290ba95e9..2a0da8b3cc 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -31,6 +31,9 @@ except ModuleNotFoundError: class TestFunctional(unittest.TestCase): """Functional tests for checking that patman behaves correctly""" + leb = (b'Lord Edmund Blackadd\xc3\xabr '. + decode('utf-8')) + def setUp(self): self.tmpdir = tempfile.mkdtemp(prefix='patman.') self.gitdir = os.path.join(self.tmpdir, 'git') @@ -177,8 +180,6 @@ class TestFunctional(unittest.TestCase): stefan = b'Stefan Br\xc3\xbcns '.decode('utf-8') rick = 'Richard III ' mel = b'Lord M\xc3\xablchett '.decode('utf-8') - leb = (b'Lond Edmund Blackadd\xc3\xabr +Reviewed-by: Mary Bloggs +Tested-by: %s +''' % self.leb + pstrm = PatchStream.process_text(text) + self.assertEqual(pstrm.commit.rtags, { + 'Reviewed-by': {'Mary Bloggs ', + 'Joe Bloggs '}, + 'Tested-by': {self.leb}}) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index cf591b2757..d6f6ae9251 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -5,6 +5,7 @@ """Handles parsing a stream of commits/emails from 'git log' or other source""" import datetime +import io import math import os import re @@ -81,6 +82,28 @@ class PatchStream: self.state = STATE_MSG_HEADER # What state are we in? self.commit = None # Current commit + @staticmethod + def process_text(text, is_comment=False): + """Process some text through this class using a default Commit/Series + + Args: + text (str): Text to parse + is_comment (bool): True if this is a comment rather than a patch. + If True, PatchStream doesn't expect a patch subject at the + start, but jumps straight into the body + + Returns: + PatchStream: object with results + """ + pstrm = PatchStream(Series()) + pstrm.commit = commit.Commit(None) + infd = io.StringIO(text) + outfd = io.StringIO() + if is_comment: + pstrm.state = STATE_PATCH_HEADER + pstrm.process_stream(infd, outfd) + return pstrm + def _add_warn(self, warn): """Add a new warning to report to the user about the current commit From 4af99874562f2ae26e84a9904009c7ce121d540d Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:28 -0600 Subject: [PATCH 34/44] patman: Add some tests for warnings Add tests that check that warnings are generated when expected. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 111 +++++++++++++++++++++++++++++++++--- tools/patman/main.py | 16 +++++- tools/patman/patchstream.py | 2 +- 3 files changed, 116 insertions(+), 13 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 2a0da8b3cc..02d46ae5f7 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -33,6 +33,9 @@ class TestFunctional(unittest.TestCase): """Functional tests for checking that patman behaves correctly""" leb = (b'Lord Edmund Blackadd\xc3\xabr '. decode('utf-8')) + fred = 'Fred Bloggs ' + joe = 'Joe Bloggs ' + mary = 'Mary Bloggs ' def setUp(self): self.tmpdir = tempfile.mkdtemp(prefix='patman.') @@ -180,7 +183,6 @@ class TestFunctional(unittest.TestCase): stefan = b'Stefan Br\xc3\xbcns '.decode('utf-8') rick = 'Richard III ' mel = b'Lord M\xc3\xablchett '.decode('utf-8') - fred = 'Fred Bloggs ' add_maintainers = [stefan, rick] dry_run = True in_reply_to = mel @@ -189,7 +191,7 @@ class TestFunctional(unittest.TestCase): 'fdt': ['simon'], 'u-boot': ['u-boot@lists.denx.de'], 'simon': [self.leb], - 'fred': [fred], + 'fred': [self.fred], } text = self._get_text('test01.txt') @@ -231,7 +233,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual('Prefix:\t RFC', lines[line + 3]) self.assertEqual('Cover: 4 lines', lines[line + 4]) line += 5 - self.assertEqual(' Cc: %s' % fred, lines[line + 0]) + self.assertEqual(' Cc: %s' % self.fred, lines[line + 0]) self.assertEqual(' Cc: %s' % tools.FromUnicode(self.leb), lines[line + 1]) self.assertEqual(' Cc: %s' % tools.FromUnicode(mel), @@ -248,7 +250,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual(('%s %s\0%s' % (args[0], rick, stefan)), tools.ToUnicode(cc_lines[0])) self.assertEqual( - '%s %s\0%s\0%s\0%s' % (args[1], fred, self.leb, rick, stefan), + '%s %s\0%s\0%s\0%s' % (args[1], self.fred, self.leb, rick, stefan), tools.ToUnicode(cc_lines[1])) expected = ''' @@ -487,12 +489,103 @@ complicated as possible''') text = '''This is a patch Signed-off-by: Terminator -Reviewed-by: Joe Bloggs -Reviewed-by: Mary Bloggs +Reviewed-by: %s +Reviewed-by: %s Tested-by: %s -''' % self.leb +''' % (self.joe, self.mary, self.leb) pstrm = PatchStream.process_text(text) self.assertEqual(pstrm.commit.rtags, { - 'Reviewed-by': {'Mary Bloggs ', - 'Joe Bloggs '}, + 'Reviewed-by': {self.joe, self.mary}, 'Tested-by': {self.leb}}) + + def testMissingEnd(self): + """Test a missing END tag""" + text = '''This is a patch + +Cover-letter: +This is the title +missing END after this line +Signed-off-by: Fred +''' + pstrm = PatchStream.process_text(text) + self.assertEqual(["Missing 'END' in section 'cover'"], + pstrm.commit.warn) + + def testMissingBlankLine(self): + """Test a missing blank line after a tag""" + text = '''This is a patch + +Series-changes: 2 +- First line of changes +- Missing blank line after this line +Signed-off-by: Fred +''' + pstrm = PatchStream.process_text(text) + self.assertEqual(["Missing 'blank line' in section 'Series-changes'"], + pstrm.commit.warn) + + def testInvalidCommitTag(self): + """Test an invalid Commit-xxx tag""" + text = '''This is a patch + +Commit-fred: testing +''' + pstrm = PatchStream.process_text(text) + self.assertEqual(["Line 3: Ignoring Commit-fred"], pstrm.commit.warn) + + def testSelfTest(self): + """Test a tested by tag by this user""" + test_line = 'Tested-by: %s@napier.com' % os.getenv('USER') + text = '''This is a patch + +%s +''' % test_line + pstrm = PatchStream.process_text(text) + self.assertEqual(["Ignoring '%s'" % test_line], pstrm.commit.warn) + + def testSpaceBeforeTab(self): + """Test a space before a tab""" + text = '''This is a patch + ++ \tSomething +''' + pstrm = PatchStream.process_text(text) + self.assertEqual(["Line 3/0 has space before tab"], pstrm.commit.warn) + + def testLinesAfterTest(self): + """Test detecting lines after TEST= line""" + text = '''This is a patch + +TEST=sometest +more lines +here +''' + pstrm = PatchStream.process_text(text) + self.assertEqual(["Found 2 lines after TEST="], pstrm.commit.warn) + + def testBlankLineAtEnd(self): + """Test detecting a blank line at the end of a file""" + text = '''This is a patch + +diff --git a/lib/fdtdec.c b/lib/fdtdec.c +index c072e54..942244f 100644 +--- a/lib/fdtdec.c ++++ b/lib/fdtdec.c +@@ -1200,7 +1200,8 @@ int fdtdec_setup_mem_size_base(void) + } + + gd->ram_size = (phys_size_t)(res.end - res.start + 1); +- debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size); ++ debug("%s: Initial DRAM size %llx\n", __func__, ++ (unsigned long long)gd->ram_size); ++ +diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c + +-- +2.7.4 + + ''' + pstrm = PatchStream.process_text(text) + self.assertEqual( + ["Found possible blank line(s) at end of file 'lib/fdtdec.c'"], + pstrm.commit.warn) diff --git a/tools/patman/main.py b/tools/patman/main.py index b96000807e..d1a43c44fc 100755 --- a/tools/patman/main.py +++ b/tools/patman/main.py @@ -86,6 +86,8 @@ AddCommonArgs(send) send.add_argument('patchfiles', nargs='*') test_parser = subparsers.add_parser('test', help='Run tests') +test_parser.add_argument('testname', type=str, default=None, nargs='?', + help="Specify the test to run") AddCommonArgs(test_parser) # Parse options twice: first to get the project and second to handle @@ -111,15 +113,23 @@ if args.cmd == 'test': sys.argv = [sys.argv[0]] result = unittest.TestResult() + suite = unittest.TestSuite() + loader = unittest.TestLoader() for module in (test_checkpatch.TestPatch, func_test.TestFunctional): - suite = unittest.TestLoader().loadTestsFromTestCase(module) - suite.run(result) + if args.testname: + try: + suite.addTests(loader.loadTestsFromName(args.testname, module)) + except AttributeError: + continue + else: + suite.addTests(loader.loadTestsFromTestCase(module)) + suite.run(result) for module in ['gitutil', 'settings', 'terminal']: suite = doctest.DocTestSuite(module) suite.run(result) - sys.exit(test_util.ReportResult('patman', None, result)) + sys.exit(test_util.ReportResult('patman', args.testname, result)) # Process commits, produce patches files, check them, email them elif args.cmd == 'send': diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index d6f6ae9251..1cb4d6ed0d 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -419,7 +419,7 @@ class PatchStream: # Remove Tested-by self, since few will take much notice if (rtag_type == 'Tested-by' and who.find(os.getenv('USER') + '@') != -1): - self._add_warn("Ignoring %s" % line) + self._add_warn("Ignoring '%s'" % line) elif rtag_type == 'Patch-cc': self.commit.AddCc(who.split(',')) else: From 8c17f8c5d21842bc95ca6e27680009b6bc650b0f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:29 -0600 Subject: [PATCH 35/44] patman: Convert testBasic() to use an interator On balance it is easier to use an iterator here, particularly if we need to insert lines due to new functionality. The only niggle is the need to keep the previous iterator value around in one case. Convert this test to use iter(). Signed-off-by: Simon Glass --- tools/patman/func_test.py | 55 +++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 02d46ae5f7..b39e3f671d 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -212,40 +212,39 @@ class TestFunctional(unittest.TestCase): cc_lines = open(cc_file, encoding='utf-8').read().splitlines() os.remove(cc_file) - lines = out[0].getvalue().splitlines() - self.assertEqual('Cleaned %s patches' % len(series.commits), lines[0]) - self.assertEqual('Change log missing for v2', lines[1]) - self.assertEqual('Change log missing for v3', lines[2]) - self.assertEqual('Change log for unknown version v4', lines[3]) - self.assertEqual("Alias 'pci' not found", lines[4]) - self.assertIn('Dry run', lines[5]) - self.assertIn('Send a total of %d patches' % count, lines[7]) - line = 8 - for i in range(len(series.commits)): - self.assertEqual(' %s' % args[i], lines[line + 0]) - line += 1 - while 'Cc:' in lines[line]: - line += 1 - self.assertEqual('To: u-boot@lists.denx.de', lines[line]) - self.assertEqual('Cc: %s' % tools.FromUnicode(stefan), - lines[line + 1]) - self.assertEqual('Version: 3', lines[line + 2]) - self.assertEqual('Prefix:\t RFC', lines[line + 3]) - self.assertEqual('Cover: 4 lines', lines[line + 4]) - line += 5 - self.assertEqual(' Cc: %s' % self.fred, lines[line + 0]) + lines = iter(out[0].getvalue().splitlines()) + self.assertEqual('Cleaned %s patches' % len(series.commits), + next(lines)) + self.assertEqual('Change log missing for v2', next(lines)) + self.assertEqual('Change log missing for v3', next(lines)) + self.assertEqual('Change log for unknown version v4', next(lines)) + self.assertEqual("Alias 'pci' not found", next(lines)) + self.assertIn('Dry run', next(lines)) + self.assertEqual('', next(lines)) + self.assertIn('Send a total of %d patches' % count, next(lines)) + prev = next(lines) + for i, commit in enumerate(series.commits): + self.assertEqual(' %s' % args[i], prev) + while True: + prev = next(lines) + if 'Cc:' not in prev: + break + self.assertEqual('To: u-boot@lists.denx.de', prev) + self.assertEqual('Cc: %s' % tools.FromUnicode(stefan), next(lines)) + self.assertEqual('Version: 3', next(lines)) + self.assertEqual('Prefix:\t RFC', next(lines)) + self.assertEqual('Cover: 4 lines', next(lines)) + self.assertEqual(' Cc: %s' % self.fred, next(lines)) self.assertEqual(' Cc: %s' % tools.FromUnicode(self.leb), - lines[line + 1]) - self.assertEqual(' Cc: %s' % tools.FromUnicode(mel), - lines[line + 2]) - self.assertEqual(' Cc: %s' % rick, lines[line + 3]) + next(lines)) + self.assertEqual(' Cc: %s' % tools.FromUnicode(mel), next(lines)) + self.assertEqual(' Cc: %s' % rick, next(lines)) expected = ('Git command: git send-email --annotate ' '--in-reply-to="%s" --to "u-boot@lists.denx.de" ' '--cc "%s" --cc-cmd "%s --cc-cmd %s" %s %s' % (in_reply_to, stefan, sys.argv[0], cc_file, cover_fname, ' '.join(args))) - line += 4 - self.assertEqual(expected, tools.ToUnicode(lines[line])) + self.assertEqual(expected, tools.ToUnicode(next(lines))) self.assertEqual(('%s %s\0%s' % (args[0], rick, stefan)), tools.ToUnicode(cc_lines[0])) From 9994baadc0317e1a6a252b2b873f47f9c3390b57 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:30 -0600 Subject: [PATCH 36/44] patman: Fix spelling of plural for warning Tidy up the extra 's' when there is only a single warning. Fix the empty print statement also. Signed-off-by: Simon Glass --- tools/patman/patchstream.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 1cb4d6ed0d..b3a25d59d1 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -665,12 +665,13 @@ def fix_patches(series, fnames): cmt.count = count result = fix_patch(backup_dir, fname, series, cmt) if result: - print('%d warnings for %s:' % (len(result), fname)) + print('%d warning%s for %s:' % + (len(result), 's' if len(result) > 1 else '', fname)) for warn in result: - print('\t', warn) - print + print('\t%s' % warn) + print() count += 1 - print('Cleaned %d patches' % count) + print('Cleaned %d patch%s' % (count, 'es' if count > 1 else '')) def insert_cover_letter(fname, series, count): """Inserts a cover letter with the required info into patch 0 From b5e188131f048ad57420545c7e867e80d60b835b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:31 -0600 Subject: [PATCH 37/44] patman: Don't ignore lines starting with hash These lines can indicate a continuation of an error and should not be ignored. Fix this. Fixes: 666eb15e923 ("patman: Handle checkpatch output with notes and code") Signed-off-by: Simon Glass --- tools/patman/checkpatch.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py index 263bac3fc9..98d962cd50 100644 --- a/tools/patman/checkpatch.py +++ b/tools/patman/checkpatch.py @@ -95,6 +95,7 @@ def CheckPatch(fname, verbose=False, show_types=False): re_check = re.compile('CHECK:%s (.*)' % type_name) re_file = re.compile('#\d+: FILE: ([^:]*):(\d+):') re_note = re.compile('NOTE: (.*)') + re_new_file = re.compile('new file mode .*') indent = ' ' * 6 for line in result.stdout.splitlines(): if verbose: @@ -111,8 +112,10 @@ def CheckPatch(fname, verbose=False, show_types=False): # Skip lines which quote code if line.startswith(indent): continue - # Skip code quotes and # - if line.startswith('+') or line.startswith('#'): + # Skip code quotes + if line.startswith('+'): + continue + if re_new_file.match(line): continue match = re_stats_full.match(line) if not match: From a12ad7c94064759f5be02b879f3f52ed5111335f Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:32 -0600 Subject: [PATCH 38/44] patman: Allow showing a Commit as a string Use the subject of the Commit object when printing it out. Signed-off-by: Simon Glass --- tools/patman/commit.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/patman/commit.py b/tools/patman/commit.py index e49bf87dfc..5bf2b94029 100644 --- a/tools/patman/commit.py +++ b/tools/patman/commit.py @@ -41,6 +41,9 @@ class Commit: self.rtags = collections.defaultdict(set) self.warn = [] + def __str__(self): + return self.subject + def AddChange(self, version, info): """Add a new change line to the change list for a version. From b3348522b753450be9e442452bf42aaa032d15d1 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:33 -0600 Subject: [PATCH 39/44] patman: Improve handling of files Sometimes warnings are associated with a file and sometimes with the patch as a whole. Update the regular expression to handle both cases, even in emacs mode. Also add support for detecting new files. Signed-off-by: Simon Glass --- tools/patman/checkpatch.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py index 98d962cd50..63a8e37e8c 100644 --- a/tools/patman/checkpatch.py +++ b/tools/patman/checkpatch.py @@ -93,7 +93,7 @@ def CheckPatch(fname, verbose=False, show_types=False): re_error = re.compile('ERROR:%s (.*)' % type_name) re_warning = re.compile(emacs_prefix + 'WARNING:%s (.*)' % type_name) re_check = re.compile('CHECK:%s (.*)' % type_name) - re_file = re.compile('#\d+: FILE: ([^:]*):(\d+):') + re_file = re.compile('#(\d+): (FILE: ([^:]*):(\d+):)?') re_note = re.compile('NOTE: (.*)') re_new_file = re.compile('new file mode .*') indent = ' ' * 6 @@ -153,8 +153,13 @@ def CheckPatch(fname, verbose=False, show_types=False): item['msg'] = check_match.group(2) item['type'] = 'check' elif file_match: - item['file'] = file_match.group(1) - item['line'] = int(file_match.group(2)) + err_fname = file_match.group(3) + if err_fname: + item['file'] = err_fname + item['line'] = int(file_match.group(4)) + else: + item['file'] = '' + item['line'] = int(file_match.group(1)) elif subject_match: item['file'] = '' item['line'] = None From be051c0c7741d67f5093f6b61b64c45eb200235b Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:34 -0600 Subject: [PATCH 40/44] patman: Detect missing upstream in CountCommitsToBranch At present if we fail to find the upstream then the error output is piped to wc, resulting in bogus results. Avoid the pipe and check the output directly. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 19 +++++++++++++++++++ tools/patman/gitutil.py | 10 +++++++--- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index b39e3f671d..cce3905c09 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -588,3 +588,22 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual( ["Found possible blank line(s) at end of file 'lib/fdtdec.c'"], pstrm.commit.warn) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testNoUpstream(self): + """Test CountCommitsToBranch when there is no upstream""" + repo = self.make_git_tree() + target = repo.lookup_reference('refs/heads/base') + self.repo.checkout(target, strategy=pygit2.GIT_CHECKOUT_FORCE) + + # Check that it can detect the current branch + try: + orig_dir = os.getcwd() + os.chdir(self.gitdir) + with self.assertRaises(ValueError) as exc: + gitutil.CountCommitsToBranch(None) + self.assertIn( + "Failed to determine upstream: fatal: no upstream configured for branch 'base'", + str(exc.exception)) + finally: + os.chdir(orig_dir) diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py index 27a0a9fbc1..3a2366bcf5 100644 --- a/tools/patman/gitutil.py +++ b/tools/patman/gitutil.py @@ -66,9 +66,13 @@ def CountCommitsToBranch(branch): rev_range = '%s..%s' % (us, branch) else: rev_range = '@{upstream}..' - pipe = [LogCmd(rev_range, oneline=True), ['wc', '-l']] - stdout = command.RunPipe(pipe, capture=True, oneline=True).stdout - patch_count = int(stdout) + pipe = [LogCmd(rev_range, oneline=True)] + result = command.RunPipe(pipe, capture=True, capture_stderr=True, + oneline=True, raise_on_error=False) + if result.return_code: + raise ValueError('Failed to determine upstream: %s' % + result.stderr.strip()) + patch_count = len(result.stdout.splitlines()) return patch_count def NameRevision(commit_hash): From dc6df972c9ee2afefd937bee3771865012daccef Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:35 -0600 Subject: [PATCH 41/44] patman: Support checking for review tags in patchwork MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before sending out a new version of a series for review, it is important to add any review tags (e.g. Reviewed-by, Acked-by) collected by patchwork. Otherwise people waste time reviewing the same patch repeatedly, become frustrated and stop reviewing your patches. To help with this, add a new 'status' subcommand that checks patchwork for review tags, showing those which are not present in the local branch. This allows users to see what new review tags have been received and then add them. Sample output: $ patman status 1 Subject 1 Reviewed-by: Joe Bloggs 2 Subject 2 Tested-by: Lord Edmund Blackaddër Reviewed-by: Fred Bloggs + Reviewed-by: Mary Bloggs 1 new response available in patchwork The '+' indicates a new tag. Colours are used to make it easier to read. Signed-off-by: Simon Glass --- tools/patman/README | 34 ++++ tools/patman/control.py | 46 +++++ tools/patman/func_test.py | 315 +++++++++++++++++++++++++++++++++ tools/patman/main.py | 18 ++ tools/patman/status.py | 356 ++++++++++++++++++++++++++++++++++++++ tools/patman/terminal.py | 21 ++- 6 files changed, 784 insertions(+), 6 deletions(-) create mode 100644 tools/patman/status.py diff --git a/tools/patman/README b/tools/patman/README index 6664027ed7..46b8e251ca 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -11,6 +11,8 @@ This tool is a Python script which: - Runs the patches through checkpatch.pl and its own checks - Optionally emails them out to selected people +It also shows review tags from Patchwork so you can update your local patches. + It is intended to automate patch creation and make it a less error-prone process. It is useful for U-Boot and Linux work so far, since it uses the checkpatch.pl script. @@ -352,6 +354,38 @@ These people will get the cover letter even if they are not on the To/Cc list for any of the patches. +Patchwork Integration +===================== + +Patman has a very basic integration with Patchwork. If you point patman to +your series on patchwork it can show you what new reviews have appears since +you sent your series. + +To set this up, add a Series-link tag to one of the commits in your series +(see above). + +Then you can type + + patman status + +and patman will show you each patch and what review tags have been collected, +for example: + +... + 21 x86: mtrr: Update the command to use the new mtrr + Reviewed-by: Wolfgang Wallner + + Reviewed-by: Bin Meng + 22 x86: mtrr: Restructure so command execution is in + Reviewed-by: Wolfgang Wallner + + Reviewed-by: Bin Meng +... + +This shows that patch 21 and 22 were sent out with one review but have since +attracted another review each. If the series needs changes, you can update +these commits with the new review tag before sending the next version of the +series. + + Example Work Flow ================= diff --git a/tools/patman/control.py b/tools/patman/control.py index 6555a4018a..7a5469add1 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -175,3 +175,49 @@ def send(args): its_a_go, args.ignore_bad_tags, args.add_maintainers, args.limit, args.dry_run, args.in_reply_to, args.thread, args.smtp_server) + +def patchwork_status(branch, count, start, end): + """Check the status of patches in patchwork + + This finds the series in patchwork using the Series-link tag, checks for new + comments / review tags and displays them + + Args: + branch (str): Branch to create patches from (None = current) + count (int): Number of patches to produce, or -1 to produce patches for + the current branch back to the upstream commit + start (int): Start partch to use (0=first / top of branch) + end (int): End patch to use (0=last one in series, 1=one before that, + etc.) + + Raises: + ValueError: if the branch has no Series-link value + """ + if count == -1: + # Work out how many patches to send if we can + count = (gitutil.CountCommitsToBranch(branch) - start) + + series = patchstream.get_metadata(branch, start, count - end) + warnings = 0 + for cmt in series.commits: + if cmt.warn: + print('%d warnings for %s:' % (len(cmt.warn), cmt.hash)) + for warn in cmt.warn: + print('\t', warn) + warnings += 1 + print + if warnings: + raise ValueError('Please fix warnings before running status') + links = series.get('links') + if not links: + raise ValueError("Branch has no Series-links value") + + # Find the link without a version number (we don't support versions yet) + found = [link for link in links.split() if not ':' in link] + if not found: + raise ValueError('Series-links has no current version (without :)') + + # Import this here to avoid failing on other commands if the dependencies + # are not present + from patman import status + status.check_patchwork_status(series, found[0]) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index cce3905c09..722844e15d 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -13,10 +13,13 @@ import sys import tempfile import unittest + +from patman.commit import Commit from patman import control from patman import gitutil from patman import patchstream from patman.patchstream import PatchStream +from patman.series import Series from patman import settings from patman import terminal from patman import tools @@ -25,6 +28,7 @@ from patman.test_util import capture_sys_output try: import pygit2 HAVE_PYGIT2 = True + from patman import status except ModuleNotFoundError: HAVE_PYGIT2 = False @@ -36,6 +40,8 @@ class TestFunctional(unittest.TestCase): fred = 'Fred Bloggs ' joe = 'Joe Bloggs ' mary = 'Mary Bloggs ' + commits = None + patches = None def setUp(self): self.tmpdir = tempfile.mkdtemp(prefix='patman.') @@ -44,6 +50,7 @@ class TestFunctional(unittest.TestCase): def tearDown(self): shutil.rmtree(self.tmpdir) + terminal.SetPrintTestMode(False) @staticmethod def _get_path(fname): @@ -607,3 +614,311 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c str(exc.exception)) finally: os.chdir(orig_dir) + + @staticmethod + def _fake_patchwork(subpath): + """Fake Patchwork server for the function below + + This handles accessing a series, providing a list consisting of a + single patch + """ + re_series = re.match(r'series/(\d*)/$', subpath) + if re_series: + series_num = re_series.group(1) + if series_num == '1234': + return {'patches': [ + {'id': '1', 'name': 'Some patch'}]} + raise ValueError('Fake Patchwork does not understand: %s' % subpath) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testStatusMismatch(self): + """Test Patchwork patches not matching the series""" + series = Series() + + with capture_sys_output() as (_, err): + status.collect_patches(series, 1234, self._fake_patchwork) + self.assertIn('Warning: Patchwork reports 1 patches, series has 0', + err.getvalue()) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testStatusReadPatch(self): + """Test handling a single patch in Patchwork""" + series = Series() + series.commits = [Commit('abcd')] + + patches = status.collect_patches(series, 1234, self._fake_patchwork) + self.assertEqual(1, len(patches)) + patch = patches[0] + self.assertEqual('1', patch.id) + self.assertEqual('Some patch', patch.raw_subject) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testParseSubject(self): + """Test parsing of the patch subject""" + patch = status.Patch('1') + + # Simple patch not in a series + patch.parse_subject('Testing') + self.assertEqual('Testing', patch.raw_subject) + self.assertEqual('Testing', patch.subject) + self.assertEqual(1, patch.seq) + self.assertEqual(1, patch.count) + self.assertEqual(None, patch.prefix) + self.assertEqual(None, patch.version) + + # First patch in a series + patch.parse_subject('[1/2] Testing') + self.assertEqual('[1/2] Testing', patch.raw_subject) + self.assertEqual('Testing', patch.subject) + self.assertEqual(1, patch.seq) + self.assertEqual(2, patch.count) + self.assertEqual(None, patch.prefix) + self.assertEqual(None, patch.version) + + # Second patch in a series + patch.parse_subject('[2/2] Testing') + self.assertEqual('Testing', patch.subject) + self.assertEqual(2, patch.seq) + self.assertEqual(2, patch.count) + self.assertEqual(None, patch.prefix) + self.assertEqual(None, patch.version) + + # RFC patch + patch.parse_subject('[RFC,3/7] Testing') + self.assertEqual('Testing', patch.subject) + self.assertEqual(3, patch.seq) + self.assertEqual(7, patch.count) + self.assertEqual('RFC', patch.prefix) + self.assertEqual(None, patch.version) + + # Version patch + patch.parse_subject('[v2,3/7] Testing') + self.assertEqual('Testing', patch.subject) + self.assertEqual(3, patch.seq) + self.assertEqual(7, patch.count) + self.assertEqual(None, patch.prefix) + self.assertEqual('v2', patch.version) + + # All fields + patch.parse_subject('[RESEND,v2,3/7] Testing') + self.assertEqual('Testing', patch.subject) + self.assertEqual(3, patch.seq) + self.assertEqual(7, patch.count) + self.assertEqual('RESEND', patch.prefix) + self.assertEqual('v2', patch.version) + + # RFC only + patch.parse_subject('[RESEND] Testing') + self.assertEqual('Testing', patch.subject) + self.assertEqual(1, patch.seq) + self.assertEqual(1, patch.count) + self.assertEqual('RESEND', patch.prefix) + self.assertEqual(None, patch.version) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testCompareSeries(self): + """Test operation of compare_with_series()""" + commit1 = Commit('abcd') + commit1.subject = 'Subject 1' + commit2 = Commit('ef12') + commit2.subject = 'Subject 2' + commit3 = Commit('3456') + commit3.subject = 'Subject 2' + + patch1 = status.Patch('1') + patch1.subject = 'Subject 1' + patch2 = status.Patch('2') + patch2.subject = 'Subject 2' + patch3 = status.Patch('3') + patch3.subject = 'Subject 2' + + series = Series() + series.commits = [commit1] + patches = [patch1] + patch_for_commit, commit_for_patch, warnings = ( + status.compare_with_series(series, patches)) + self.assertEqual(1, len(patch_for_commit)) + self.assertEqual(patch1, patch_for_commit[0]) + self.assertEqual(1, len(commit_for_patch)) + self.assertEqual(commit1, commit_for_patch[0]) + + series.commits = [commit1] + patches = [patch1, patch2] + patch_for_commit, commit_for_patch, warnings = ( + status.compare_with_series(series, patches)) + self.assertEqual(1, len(patch_for_commit)) + self.assertEqual(patch1, patch_for_commit[0]) + self.assertEqual(1, len(commit_for_patch)) + self.assertEqual(commit1, commit_for_patch[0]) + self.assertEqual(["Cannot find commit for patch 2 ('Subject 2')"], + warnings) + + series.commits = [commit1, commit2] + patches = [patch1] + patch_for_commit, commit_for_patch, warnings = ( + status.compare_with_series(series, patches)) + self.assertEqual(1, len(patch_for_commit)) + self.assertEqual(patch1, patch_for_commit[0]) + self.assertEqual(1, len(commit_for_patch)) + self.assertEqual(commit1, commit_for_patch[0]) + self.assertEqual(["Cannot find patch for commit 2 ('Subject 2')"], + warnings) + + series.commits = [commit1, commit2, commit3] + patches = [patch1, patch2] + patch_for_commit, commit_for_patch, warnings = ( + status.compare_with_series(series, patches)) + self.assertEqual(2, len(patch_for_commit)) + self.assertEqual(patch1, patch_for_commit[0]) + self.assertEqual(patch2, patch_for_commit[1]) + self.assertEqual(1, len(commit_for_patch)) + self.assertEqual(commit1, commit_for_patch[0]) + self.assertEqual(["Cannot find patch for commit 3 ('Subject 2')", + "Multiple commits match patch 2 ('Subject 2'):\n" + ' Subject 2\n Subject 2'], + warnings) + + series.commits = [commit1, commit2] + patches = [patch1, patch2, patch3] + patch_for_commit, commit_for_patch, warnings = ( + status.compare_with_series(series, patches)) + self.assertEqual(1, len(patch_for_commit)) + self.assertEqual(patch1, patch_for_commit[0]) + self.assertEqual(2, len(commit_for_patch)) + self.assertEqual(commit1, commit_for_patch[0]) + self.assertEqual(["Multiple patches match commit 2 ('Subject 2'):\n" + ' Subject 2\n Subject 2', + "Cannot find commit for patch 3 ('Subject 2')"], + warnings) + + def _fake_patchwork2(self, subpath): + """Fake Patchwork server for the function below + + This handles accessing series, patches and comments, providing the data + in self.patches to the caller + """ + re_series = re.match(r'series/(\d*)/$', subpath) + re_patch = re.match(r'patches/(\d*)/$', subpath) + re_comments = re.match(r'patches/(\d*)/comments/$', subpath) + if re_series: + series_num = re_series.group(1) + if series_num == '1234': + return {'patches': self.patches} + elif re_patch: + patch_num = int(re_patch.group(1)) + patch = self.patches[patch_num - 1] + return patch + elif re_comments: + patch_num = int(re_comments.group(1)) + patch = self.patches[patch_num - 1] + return patch.comments + raise ValueError('Fake Patchwork does not understand: %s' % subpath) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testFindNewResponses(self): + """Test operation of find_new_responses()""" + commit1 = Commit('abcd') + commit1.subject = 'Subject 1' + commit2 = Commit('ef12') + commit2.subject = 'Subject 2' + + patch1 = status.Patch('1') + patch1.parse_subject('[1/2] Subject 1') + patch1.name = patch1.raw_subject + patch1.content = 'This is my patch content' + comment1a = {'content': 'Reviewed-by: %s\n' % self.joe} + + patch1.comments = [comment1a] + + patch2 = status.Patch('2') + patch2.parse_subject('[2/2] Subject 2') + patch2.name = patch2.raw_subject + patch2.content = 'Some other patch content' + comment2a = { + 'content': 'Reviewed-by: %s\nTested-by: %s\n' % + (self.mary, self.leb)} + comment2b = {'content': 'Reviewed-by: %s' % self.fred} + patch2.comments = [comment2a, comment2b] + + # This test works by setting up commits and patch for use by the fake + # Rest API function _fake_patchwork2(). It calls various functions in + # the status module after setting up tags in the commits, checking that + # things behaves as expected + self.commits = [commit1, commit2] + self.patches = [patch1, patch2] + count = 2 + new_rtag_list = [None] * count + + # Check that the tags are picked up on the first patch + status.find_new_responses(new_rtag_list, 0, commit1, patch1, + self._fake_patchwork2) + self.assertEqual(new_rtag_list[0], {'Reviewed-by': {self.joe}}) + + # Now the second patch + status.find_new_responses(new_rtag_list, 1, commit2, patch2, + self._fake_patchwork2) + self.assertEqual(new_rtag_list[1], { + 'Reviewed-by': {self.mary, self.fred}, + 'Tested-by': {self.leb}}) + + # Now add some tags to the commit, which means they should not appear as + # 'new' tags when scanning comments + new_rtag_list = [None] * count + commit1.rtags = {'Reviewed-by': {self.joe}} + status.find_new_responses(new_rtag_list, 0, commit1, patch1, + self._fake_patchwork2) + self.assertEqual(new_rtag_list[0], {}) + + # For the second commit, add Ed and Fred, so only Mary should be left + commit2.rtags = { + 'Tested-by': {self.leb}, + 'Reviewed-by': {self.fred}} + status.find_new_responses(new_rtag_list, 1, commit2, patch2, + self._fake_patchwork2) + self.assertEqual(new_rtag_list[1], {'Reviewed-by': {self.mary}}) + + # Check that the output patches expectations: + # 1 Subject 1 + # Reviewed-by: Joe Bloggs + # 2 Subject 2 + # Tested-by: Lord Edmund Blackaddër + # Reviewed-by: Fred Bloggs + # + Reviewed-by: Mary Bloggs + # 1 new response available in patchwork + + series = Series() + series.commits = [commit1, commit2] + terminal.SetPrintTestMode() + status.check_patchwork_status(series, '1234', self._fake_patchwork2) + lines = iter(terminal.GetPrintTestLines()) + col = terminal.Color() + self.assertEqual(terminal.PrintLine(' 1 Subject 1', col.BLUE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' Reviewed-by: ', col.GREEN, newline=False, + bright=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.joe, col.WHITE, bright=False), + next(lines)) + + self.assertEqual(terminal.PrintLine(' 2 Subject 2', col.BLUE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' Tested-by: ', col.GREEN, newline=False, + bright=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False), + next(lines)) + self.assertEqual( + terminal.PrintLine(' Reviewed-by: ', col.GREEN, newline=False, + bright=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.fred, col.WHITE, bright=False), + next(lines)) + self.assertEqual( + terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.mary, col.WHITE), + next(lines)) + self.assertEqual(terminal.PrintLine( + '1 new response available in patchwork', None), next(lines)) diff --git a/tools/patman/main.py b/tools/patman/main.py index d1a43c44fc..7f4ae1125a 100755 --- a/tools/patman/main.py +++ b/tools/patman/main.py @@ -90,6 +90,10 @@ test_parser.add_argument('testname', type=str, default=None, nargs='?', help="Specify the test to run") AddCommonArgs(test_parser) +status = subparsers.add_parser('status', + help='Check status of patches in patchwork') +AddCommonArgs(status) + # Parse options twice: first to get the project and second to handle # defaults properly (which depends on project). argv = sys.argv[1:] @@ -157,3 +161,17 @@ elif args.cmd == 'send': else: control.send(args) + +# Check status of patches in patchwork +elif args.cmd == 'status': + ret_code = 0 + try: + control.patchwork_status(args.branch, args.count, args.start, args.end) + except Exception as e: + terminal.Print('patman: %s: %s' % (type(e).__name__, e), + colour=terminal.Color.RED) + if args.debug: + print() + traceback.print_exc() + ret_code = 1 + sys.exit(ret_code) diff --git a/tools/patman/status.py b/tools/patman/status.py new file mode 100644 index 0000000000..f41b2d4c77 --- /dev/null +++ b/tools/patman/status.py @@ -0,0 +1,356 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# Copyright 2020 Google LLC +# +"""Talks to the patchwork service to figure out what patches have been reviewed +and commented on. +""" + +import collections +import concurrent.futures +from itertools import repeat +import re +import requests + +from patman.patchstream import PatchStream +from patman import terminal +from patman import tout + +# Patches which are part of a multi-patch series are shown with a prefix like +# [prefix, version, sequence], for example '[RFC, v2, 3/5]'. All but the last +# part is optional. This decodes the string into groups. For single patches +# the [] part is not present: +# Groups: (ignore, ignore, ignore, prefix, version, sequence, subject) +RE_PATCH = re.compile(r'(\[(((.*),)?(.*),)?(.*)\]\s)?(.*)$') + +# This decodes the sequence string into a patch number and patch count +RE_SEQ = re.compile(r'(\d+)/(\d+)') + +def to_int(vals): + """Convert a list of strings into integers, using 0 if not an integer + + Args: + vals (list): List of strings + + Returns: + list: List of integers, one for each input string + """ + out = [int(val) if val.isdigit() else 0 for val in vals] + return out + + +class Patch(dict): + """Models a patch in patchwork + + This class records information obtained from patchwork + + Some of this information comes from the 'Patch' column: + + [RFC,v2,1/3] dm: Driver and uclass changes for tiny-dm + + This shows the prefix, version, seq, count and subject. + + The other properties come from other columns in the display. + + Properties: + pid (str): ID of the patch (typically an integer) + seq (int): Sequence number within series (1=first) parsed from sequence + string + count (int): Number of patches in series, parsed from sequence string + raw_subject (str): Entire subject line, e.g. + "[1/2,v2] efi_loader: Sort header file ordering" + prefix (str): Prefix string or None (e.g. 'RFC') + version (str): Version string or None (e.g. 'v2') + raw_subject (str): Raw patch subject + subject (str): Patch subject with [..] part removed (same as commit + subject) + """ + def __init__(self, pid): + super().__init__() + self.id = pid # Use 'id' to match what the Rest API provides + self.seq = None + self.count = None + self.prefix = None + self.version = None + self.raw_subject = None + self.subject = None + + # These make us more like a dictionary + def __setattr__(self, name, value): + self[name] = value + + def __getattr__(self, name): + return self[name] + + def __hash__(self): + return hash(frozenset(self.items())) + + def __str__(self): + return self.raw_subject + + def parse_subject(self, raw_subject): + """Parse the subject of a patch into its component parts + + See RE_PATCH for details. The parsed info is placed into seq, count, + prefix, version, subject + + Args: + raw_subject (str): Subject string to parse + + Raises: + ValueError: the subject cannot be parsed + """ + self.raw_subject = raw_subject.strip() + mat = RE_PATCH.search(raw_subject.strip()) + if not mat: + raise ValueError("Cannot parse subject '%s'" % raw_subject) + self.prefix, self.version, seq_info, self.subject = mat.groups()[3:] + mat_seq = RE_SEQ.match(seq_info) if seq_info else False + if mat_seq is None: + self.version = seq_info + seq_info = None + if self.version and not self.version.startswith('v'): + self.prefix = self.version + self.version = None + if seq_info: + if mat_seq: + self.seq = int(mat_seq.group(1)) + self.count = int(mat_seq.group(2)) + else: + self.seq = 1 + self.count = 1 + +def compare_with_series(series, patches): + """Compare a list of patches with a series it came from + + This prints any problems as warnings + + Args: + series (Series): Series to compare against + patches (:type: list of Patch): list of Patch objects to compare with + + Returns: + tuple + dict: + key: Commit number (0...n-1) + value: Patch object for that commit + dict: + key: Patch number (0...n-1) + value: Commit object for that patch + """ + # Check the names match + warnings = [] + patch_for_commit = {} + all_patches = set(patches) + for seq, cmt in enumerate(series.commits): + pmatch = [p for p in all_patches if p.subject == cmt.subject] + if len(pmatch) == 1: + patch_for_commit[seq] = pmatch[0] + all_patches.remove(pmatch[0]) + elif len(pmatch) > 1: + warnings.append("Multiple patches match commit %d ('%s'):\n %s" % + (seq + 1, cmt.subject, + '\n '.join([p.subject for p in pmatch]))) + else: + warnings.append("Cannot find patch for commit %d ('%s')" % + (seq + 1, cmt.subject)) + + + # Check the names match + commit_for_patch = {} + all_commits = set(series.commits) + for seq, patch in enumerate(patches): + cmatch = [c for c in all_commits if c.subject == patch.subject] + if len(cmatch) == 1: + commit_for_patch[seq] = cmatch[0] + all_commits.remove(cmatch[0]) + elif len(cmatch) > 1: + warnings.append("Multiple commits match patch %d ('%s'):\n %s" % + (seq + 1, patch.subject, + '\n '.join([c.subject for c in cmatch]))) + else: + warnings.append("Cannot find commit for patch %d ('%s')" % + (seq + 1, patch.subject)) + + return patch_for_commit, commit_for_patch, warnings + +def call_rest_api(subpath): + """Call the patchwork API and return the result as JSON + + Args: + subpath (str): URL subpath to use + + Returns: + dict: Json result + + Raises: + ValueError: the URL could not be read + """ + url = 'https://patchwork.ozlabs.org/api/1.2/%s' % subpath + response = requests.get(url) + if response.status_code != 200: + raise ValueError("Could not read URL '%s'" % url) + return response.json() + +def collect_patches(series, series_id, rest_api=call_rest_api): + """Collect patch information about a series from patchwork + + Uses the Patchwork REST API to collect information provided by patchwork + about the status of each patch. + + Args: + series (Series): Series object corresponding to the local branch + containing the series + series_id (str): Patch series ID number + rest_api (function): API function to call to access Patchwork, for + testing + + Returns: + list: List of patches sorted by sequence number, each a Patch object + + Raises: + ValueError: if the URL could not be read or the web page does not follow + the expected structure + """ + data = rest_api('series/%s/' % series_id) + + # Get all the rows, which are patches + patch_dict = data['patches'] + count = len(patch_dict) + num_commits = len(series.commits) + if count != num_commits: + tout.Warning('Warning: Patchwork reports %d patches, series has %d' % + (count, num_commits)) + + patches = [] + + # Work through each row (patch) one at a time, collecting the information + warn_count = 0 + for pw_patch in patch_dict: + patch = Patch(pw_patch['id']) + patch.parse_subject(pw_patch['name']) + patches.append(patch) + if warn_count > 1: + tout.Warning(' (total of %d warnings)' % warn_count) + + # Sort patches by patch number + patches = sorted(patches, key=lambda x: x.seq) + return patches + +def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api): + """Find new rtags collected by patchwork that we don't know about + + This is designed to be run in parallel, once for each commit/patch + + Args: + new_rtag_list (list): New rtags are written to new_rtag_list[seq] + list, each a dict: + key: Response tag (e.g. 'Reviewed-by') + value: Set of people who gave that response, each a name/email + string + seq (int): Position in new_rtag_list to update + cmt (Commit): Commit object for this commit + patch (Patch): Corresponding Patch object for this patch + rest_api (function): API function to call to access Patchwork, for + testing + """ + if not patch: + return + + # Get the content for the patch email itself as well as all comments + data = rest_api('patches/%s/' % patch.id) + pstrm = PatchStream.process_text(data['content'], True) + + rtags = collections.defaultdict(set) + for response, people in pstrm.commit.rtags.items(): + rtags[response].update(people) + + data = rest_api('patches/%s/comments/' % patch.id) + + for comment in data: + pstrm = PatchStream.process_text(comment['content'], True) + for response, people in pstrm.commit.rtags.items(): + rtags[response].update(people) + + # Find the tags that are not in the commit + new_rtags = collections.defaultdict(set) + base_rtags = cmt.rtags + for tag, people in rtags.items(): + for who in people: + is_new = (tag not in base_rtags or + who not in base_rtags[tag]) + if is_new: + new_rtags[tag].add(who) + new_rtag_list[seq] = new_rtags + +def show_responses(rtags, indent, is_new): + """Show rtags collected + + Args: + rtags (dict): review tags to show + key: Response tag (e.g. 'Reviewed-by') + value: Set of people who gave that response, each a name/email string + indent (str): Indentation string to write before each line + is_new (bool): True if this output should be highlighted + + Returns: + int: Number of review tags displayed + """ + col = terminal.Color() + count = 0 + for tag, people in rtags.items(): + for who in people: + terminal.Print(indent + '%s %s: ' % ('+' if is_new else ' ', tag), + newline=False, colour=col.GREEN, bright=is_new) + terminal.Print(who, colour=col.WHITE, bright=is_new) + count += 1 + return count + +def check_patchwork_status(series, series_id, rest_api=call_rest_api): + """Check the status of a series on Patchwork + + This finds review tags and comments for a series in Patchwork, displaying + them to show what is new compared to the local series. + + Args: + series (Series): Series object for the existing branch + series_id (str): Patch series ID number + rest_api (function): API function to call to access Patchwork, for + testing + """ + patches = collect_patches(series, series_id, rest_api) + col = terminal.Color() + count = len(series.commits) + new_rtag_list = [None] * count + + patch_for_commit, _, warnings = compare_with_series(series, patches) + for warn in warnings: + tout.Warning(warn) + + patch_list = [patch_for_commit.get(c) for c in range(len(series.commits))] + + with concurrent.futures.ThreadPoolExecutor(max_workers=16) as executor: + futures = executor.map( + find_new_responses, repeat(new_rtag_list), range(count), + series.commits, patch_list, repeat(rest_api)) + for fresponse in futures: + if fresponse: + raise fresponse.exception() + + num_to_add = 0 + for seq, cmt in enumerate(series.commits): + patch = patch_for_commit.get(seq) + if not patch: + continue + terminal.Print('%3d %s' % (patch.seq, patch.subject[:50]), + colour=col.BLUE) + cmt = series.commits[seq] + base_rtags = cmt.rtags + new_rtags = new_rtag_list[seq] + + indent = ' ' * 2 + show_responses(base_rtags, indent, False) + num_to_add += show_responses(new_rtags, indent, True) + + terminal.Print("%d new response%s available in patchwork" % + (num_to_add, 's' if num_to_add != 1 else '')) diff --git a/tools/patman/terminal.py b/tools/patman/terminal.py index 60dbce3ce1..9be03b3a6f 100644 --- a/tools/patman/terminal.py +++ b/tools/patman/terminal.py @@ -34,14 +34,22 @@ class PrintLine: newline: True to output a newline after the text colour: Text colour to use """ - def __init__(self, text, newline, colour): + def __init__(self, text, colour, newline=True, bright=True): self.text = text self.newline = newline self.colour = colour + self.bright = bright + + def __eq__(self, other): + return (self.text == other.text and + self.newline == other.newline and + self.colour == other.colour and + self.bright == other.bright) def __str__(self): - return 'newline=%s, colour=%s, text=%s' % (self.newline, self.colour, - self.text) + return ("newline=%s, colour=%s, bright=%d, text='%s'" % + (self.newline, self.colour, self.bright, self.text)) + def CalcAsciiLen(text): """Calculate the length of a string, ignoring any ANSI sequences @@ -136,7 +144,7 @@ def Print(text='', newline=True, colour=None, limit_to_line=False, bright=True): global last_print_len if print_test_mode: - print_test_list.append(PrintLine(text, newline, colour)) + print_test_list.append(PrintLine(text, colour, newline, bright)) else: if colour: col = Color() @@ -159,11 +167,12 @@ def PrintClear(): print('\r%s\r' % (' '* last_print_len), end='', flush=True) last_print_len = None -def SetPrintTestMode(): +def SetPrintTestMode(enable=True): """Go into test mode, where all printing is recorded""" global print_test_mode - print_test_mode = True + print_test_mode = enable + GetPrintTestLines() def GetPrintTestLines(): """Get a list of all lines output through Print() From 8f9ba3ab56c49880fb13fc483493b635f787627c Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:36 -0600 Subject: [PATCH 42/44] patman: Support updating a branch with review tags It is tedious to add review tags into the local branch and errors can sometimes be made. Add an option to create a new branch with the review tags obtained from patchwork. Signed-off-by: Simon Glass --- tools/patman/README | 19 +++++- tools/patman/control.py | 9 ++- tools/patman/func_test.py | 127 +++++++++++++++++++++++++++++++++++- tools/patman/main.py | 7 +- tools/patman/patchstream.py | 52 ++++++++++++++- tools/patman/status.py | 91 ++++++++++++++++++++++++-- 6 files changed, 289 insertions(+), 16 deletions(-) diff --git a/tools/patman/README b/tools/patman/README index 46b8e251ca..15da6dc33c 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -11,11 +11,13 @@ This tool is a Python script which: - Runs the patches through checkpatch.pl and its own checks - Optionally emails them out to selected people -It also shows review tags from Patchwork so you can update your local patches. +It also has some Patchwork features: +- shows review tags from Patchwork so you can update your local patches +- pulls these down into a new branch on request It is intended to automate patch creation and make it a less error-prone process. It is useful for U-Boot and Linux work so far, -since it uses the checkpatch.pl script. +since they use the checkpatch.pl script. It is configured almost entirely by tags it finds in your commits. This means that you can work on a number of different branches at @@ -385,6 +387,19 @@ attracted another review each. If the series needs changes, you can update these commits with the new review tag before sending the next version of the series. +To automatically pull into these tags into a new branch, use the -d option: + + patman status -d mtrr4 + +This will create a new 'mtrr4' branch which is the same as your current branch +but has the new review tags in it. The tags are added in alphabetic order and +are placed immediately after any existing ack/review/test/fixes tags, or at the +end. You can check that this worked with: + + patman -b mtrr4 status + +which should show that there are no new responses compared to this new branch. + Example Work Flow ================= diff --git a/tools/patman/control.py b/tools/patman/control.py index 7a5469add1..6ac258d41d 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -176,11 +176,11 @@ def send(args): args.limit, args.dry_run, args.in_reply_to, args.thread, args.smtp_server) -def patchwork_status(branch, count, start, end): +def patchwork_status(branch, count, start, end, dest_branch, force): """Check the status of patches in patchwork This finds the series in patchwork using the Series-link tag, checks for new - comments / review tags and displays them + review tags, displays then and creates a new branch with the review tags. Args: branch (str): Branch to create patches from (None = current) @@ -189,6 +189,9 @@ def patchwork_status(branch, count, start, end): start (int): Start partch to use (0=first / top of branch) end (int): End patch to use (0=last one in series, 1=one before that, etc.) + dest_branch (str): Name of new branch to create with the updated tags + (None to not create a branch) + force (bool): With dest_branch, force overwriting an existing branch Raises: ValueError: if the branch has no Series-link value @@ -220,4 +223,4 @@ def patchwork_status(branch, count, start, end): # Import this here to avoid failing on other commands if the dependencies # are not present from patman import status - status.check_patchwork_status(series, found[0]) + status.check_patchwork_status(series, found[0], branch, dest_branch, force) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 722844e15d..2e1529525e 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -403,7 +403,16 @@ with some I2C-related things in it''') self.make_commit_with_file('spi: SPI fixes', ''' SPI needs some fixes and here they are -''', 'spi.c', '''Some fixes for SPI in this + +Signed-off-by: %s + +Series-to: u-boot +Commit-notes: +title of the series +This is the cover letter for the series +with various details +END +''' % self.leb, 'spi.c', '''Some fixes for SPI in this file to make SPI work better than before''') first_target = repo.revparse_single('HEAD') @@ -889,7 +898,8 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c series = Series() series.commits = [commit1, commit2] terminal.SetPrintTestMode() - status.check_patchwork_status(series, '1234', self._fake_patchwork2) + status.check_patchwork_status(series, '1234', None, None, False, + self._fake_patchwork2) lines = iter(terminal.GetPrintTestLines()) col = terminal.Color() self.assertEqual(terminal.PrintLine(' 1 Subject 1', col.BLUE), @@ -921,4 +931,115 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual(terminal.PrintLine(self.mary, col.WHITE), next(lines)) self.assertEqual(terminal.PrintLine( - '1 new response available in patchwork', None), next(lines)) + '1 new response available in patchwork (use -d to write them to a new branch)', + None), next(lines)) + + def _fake_patchwork3(self, subpath): + """Fake Patchwork server for the function below + + This handles accessing series, patches and comments, providing the data + in self.patches to the caller + """ + re_series = re.match(r'series/(\d*)/$', subpath) + re_patch = re.match(r'patches/(\d*)/$', subpath) + re_comments = re.match(r'patches/(\d*)/comments/$', subpath) + if re_series: + series_num = re_series.group(1) + if series_num == '1234': + return {'patches': self.patches} + elif re_patch: + patch_num = int(re_patch.group(1)) + patch = self.patches[patch_num - 1] + return patch + elif re_comments: + patch_num = int(re_comments.group(1)) + patch = self.patches[patch_num - 1] + return patch.comments + raise ValueError('Fake Patchwork does not understand: %s' % subpath) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testCreateBranch(self): + """Test operation of create_branch()""" + repo = self.make_git_tree() + branch = 'first' + dest_branch = 'first2' + count = 2 + gitdir = os.path.join(self.gitdir, '.git') + + # Set up the test git tree. We use branch 'first' which has two commits + # in it + series = patchstream.get_metadata_for_list(branch, gitdir, count) + self.assertEqual(2, len(series.commits)) + + patch1 = status.Patch('1') + patch1.parse_subject('[1/2] %s' % series.commits[0].subject) + patch1.name = patch1.raw_subject + patch1.content = 'This is my patch content' + comment1a = {'content': 'Reviewed-by: %s\n' % self.joe} + + patch1.comments = [comment1a] + + patch2 = status.Patch('2') + patch2.parse_subject('[2/2] %s' % series.commits[1].subject) + patch2.name = patch2.raw_subject + patch2.content = 'Some other patch content' + comment2a = { + 'content': 'Reviewed-by: %s\nTested-by: %s\n' % + (self.mary, self.leb)} + comment2b = { + 'content': 'Reviewed-by: %s' % self.fred} + patch2.comments = [comment2a, comment2b] + + # This test works by setting up patches for use by the fake Rest API + # function _fake_patchwork3(). The fake patch comments above should + # result in new review tags that are collected and added to the commits + # created in the destination branch. + self.patches = [patch1, patch2] + count = 2 + + # Expected output: + # 1 i2c: I2C things + # + Reviewed-by: Joe Bloggs + # 2 spi: SPI fixes + # + Reviewed-by: Fred Bloggs + # + Reviewed-by: Mary Bloggs + # + Tested-by: Lord Edmund Blackaddër + # 4 new responses available in patchwork + # 4 responses added from patchwork into new branch 'first2' + # + + terminal.SetPrintTestMode() + status.check_patchwork_status(series, '1234', branch, dest_branch, + False, self._fake_patchwork3, repo) + lines = terminal.GetPrintTestLines() + self.assertEqual(12, len(lines)) + self.assertEqual( + "4 responses added from patchwork into new branch 'first2'", + lines[11].text) + + # Check that the destination branch has the new tags + new_series = patchstream.get_metadata_for_list(dest_branch, gitdir, + count) + self.assertEqual( + {'Reviewed-by': {self.joe}}, + new_series.commits[0].rtags) + self.assertEqual( + {'Tested-by': {self.leb}, + 'Reviewed-by': {self.fred, self.mary}}, + new_series.commits[1].rtags) + + # Now check the actual test of the first commit message. We expect to + # see the new tags immediately below the old ones. + stdout = patchstream.get_list(dest_branch, count=count, git_dir=gitdir) + lines = iter([line.strip() for line in stdout.splitlines() + if '-by:' in line]) + + # First patch should have the review tag + self.assertEqual('Reviewed-by: %s' % self.joe, next(lines)) + + # Second patch should have the sign-off then the tested-by and two + # reviewed-by tags + self.assertEqual('Signed-off-by: %s' % self.leb, next(lines)) + self.assertEqual('Reviewed-by: %s' % self.fred, next(lines)) + self.assertEqual('Reviewed-by: %s' % self.mary, next(lines)) + self.assertEqual('Tested-by: %s' % self.leb, next(lines)) diff --git a/tools/patman/main.py b/tools/patman/main.py index 7f4ae1125a..8f139a6e3b 100755 --- a/tools/patman/main.py +++ b/tools/patman/main.py @@ -92,6 +92,10 @@ AddCommonArgs(test_parser) status = subparsers.add_parser('status', help='Check status of patches in patchwork') +status.add_argument('-d', '--dest-branch', type=str, + help='Name of branch to create with collected responses') +status.add_argument('-f', '--force', action='store_true', + help='Force overwriting an existing branch') AddCommonArgs(status) # Parse options twice: first to get the project and second to handle @@ -166,7 +170,8 @@ elif args.cmd == 'send': elif args.cmd == 'status': ret_code = 0 try: - control.patchwork_status(args.branch, args.count, args.start, args.end) + control.patchwork_status(args.branch, args.count, args.start, args.end, + args.dest_branch, args.force) except Exception as e: terminal.Print('patman: %s: %s' % (type(e).__name__, e), colour=terminal.Color.RED) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index b3a25d59d1..a4bf08b7d8 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -551,6 +551,54 @@ class PatchStream: self.blank_count = 0 self.finalise() +def insert_tags(msg, tags_to_emit): + """Add extra tags to a commit message + + The tags are added after an existing block of tags if found, otherwise at + the end. + + Args: + msg (str): Commit message + tags_to_emit (list): List of tags to emit, each a str + + Returns: + (str) new message + """ + out = [] + done = False + emit_tags = False + for line in msg.splitlines(): + if not done: + signoff_match = RE_SIGNOFF.match(line) + tag_match = RE_TAG.match(line) + if tag_match or signoff_match: + emit_tags = True + if emit_tags and not tag_match and not signoff_match: + out += tags_to_emit + emit_tags = False + done = True + out.append(line) + if not done: + out.append('') + out += tags_to_emit + return '\n'.join(out) + +def get_list(commit_range, git_dir=None, count=None): + """Get a log of a list of comments + + This returns the output of 'git log' for the selected commits + + Args: + commit_range (str): Range of commits to count (e.g. 'HEAD..base') + git_dir (str): Path to git repositiory (None to use default) + count (int): Number of commits to list, or None for no limit + + Returns + str: String containing the contents of the git log + """ + params = gitutil.LogCmd(commit_range, reverse=True, count=count, + git_dir=git_dir) + return command.RunPipe([params], capture=True).stdout def get_metadata_for_list(commit_range, git_dir=None, count=None, series=None, allow_overwrite=False): @@ -573,9 +621,7 @@ def get_metadata_for_list(commit_range, git_dir=None, count=None, if not series: series = Series() series.allow_overwrite = allow_overwrite - params = gitutil.LogCmd(commit_range, reverse=True, count=count, - git_dir=git_dir) - stdout = command.RunPipe([params], capture=True).stdout + stdout = get_list(commit_range, git_dir, count) pst = PatchStream(series, is_log=True) for line in stdout.splitlines(): pst.process_line(line) diff --git a/tools/patman/status.py b/tools/patman/status.py index f41b2d4c77..f3a654160e 100644 --- a/tools/patman/status.py +++ b/tools/patman/status.py @@ -3,15 +3,19 @@ # Copyright 2020 Google LLC # """Talks to the patchwork service to figure out what patches have been reviewed -and commented on. +and commented on. Allows creation of a new branch based on the old but with the +review tags collected from patchwork. """ import collections import concurrent.futures from itertools import repeat import re + +import pygit2 import requests +from patman import patchstream from patman.patchstream import PatchStream from patman import terminal from patman import tout @@ -306,7 +310,73 @@ def show_responses(rtags, indent, is_new): count += 1 return count -def check_patchwork_status(series, series_id, rest_api=call_rest_api): +def create_branch(series, new_rtag_list, branch, dest_branch, overwrite, + repo=None): + """Create a new branch with review tags added + + Args: + series (Series): Series object for the existing branch + new_rtag_list (list): List of review tags to add, one for each commit, + each a dict: + key: Response tag (e.g. 'Reviewed-by') + value: Set of people who gave that response, each a name/email + string + branch (str): Existing branch to update + dest_branch (str): Name of new branch to create + overwrite (bool): True to force overwriting dest_branch if it exists + repo (pygit2.Repository): Repo to use (use None unless testing) + + Returns: + int: Total number of review tags added across all commits + + Raises: + ValueError: if the destination branch name is the same as the original + branch, or it already exists and @overwrite is False + """ + if branch == dest_branch: + raise ValueError( + 'Destination branch must not be the same as the original branch') + if not repo: + repo = pygit2.Repository('.') + count = len(series.commits) + new_br = repo.branches.get(dest_branch) + if new_br: + if not overwrite: + raise ValueError("Branch '%s' already exists (-f to overwrite)" % + dest_branch) + new_br.delete() + if not branch: + branch = 'HEAD' + target = repo.revparse_single('%s~%d' % (branch, count)) + repo.branches.local.create(dest_branch, target) + + num_added = 0 + for seq in range(count): + parent = repo.branches.get(dest_branch) + cherry = repo.revparse_single('%s~%d' % (branch, count - seq - 1)) + + repo.merge_base(cherry.oid, parent.target) + base_tree = cherry.parents[0].tree + + index = repo.merge_trees(base_tree, parent, cherry) + tree_id = index.write_tree(repo) + + lines = [] + if new_rtag_list[seq]: + for tag, people in new_rtag_list[seq].items(): + for who in people: + lines.append('%s: %s' % (tag, who)) + num_added += 1 + message = patchstream.insert_tags(cherry.message.rstrip(), + sorted(lines)) + + repo.create_commit( + parent.name, cherry.author, cherry.committer, message, tree_id, + [parent.target]) + return num_added + +def check_patchwork_status(series, series_id, branch, dest_branch, force, + rest_api=call_rest_api, test_repo=None): """Check the status of a series on Patchwork This finds review tags and comments for a series in Patchwork, displaying @@ -315,8 +385,12 @@ def check_patchwork_status(series, series_id, rest_api=call_rest_api): Args: series (Series): Series object for the existing branch series_id (str): Patch series ID number + branch (str): Existing branch to update, or None + dest_branch (str): Name of new branch to create, or None + force (bool): True to force overwriting dest_branch if it exists rest_api (function): API function to call to access Patchwork, for testing + test_repo (pygit2.Repository): Repo to use (use None unless testing) """ patches = collect_patches(series, series_id, rest_api) col = terminal.Color() @@ -352,5 +426,14 @@ def check_patchwork_status(series, series_id, rest_api=call_rest_api): show_responses(base_rtags, indent, False) num_to_add += show_responses(new_rtags, indent, True) - terminal.Print("%d new response%s available in patchwork" % - (num_to_add, 's' if num_to_add != 1 else '')) + terminal.Print("%d new response%s available in patchwork%s" % + (num_to_add, 's' if num_to_add != 1 else '', + '' if dest_branch + else ' (use -d to write them to a new branch)')) + + if dest_branch: + num_added = create_branch(series, new_rtag_list, branch, + dest_branch, force, test_repo) + terminal.Print( + "%d response%s added from patchwork into new branch '%s'" % + (num_added, 's' if num_added != 1 else '', dest_branch)) From 6b3252e230a1b8ba763883a79906d69b9aa50415 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:37 -0600 Subject: [PATCH 43/44] patman: Support parsing of review snippets Add support for parsing the contents of a patchwork 'patch' web page containing comments received from reviewers. This allows patman to show these comments in a simple 'snippets' format. A snippet is some quoted code plus some unquoted comments below it. Each review is from a unique person/email and can produce multiple snippets, one for each part of the code that attracts a comment. Show the file and line-number info at the top of each snippet if available. Signed-off-by: Simon Glass --- tools/patman/func_test.py | 83 ++++++++++++++++++++++++++++++++++++ tools/patman/patchstream.py | 85 +++++++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index 2e1529525e..bbee4b77d6 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -1043,3 +1043,86 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual('Reviewed-by: %s' % self.fred, next(lines)) self.assertEqual('Reviewed-by: %s' % self.mary, next(lines)) self.assertEqual('Tested-by: %s' % self.leb, next(lines)) + + def testParseSnippets(self): + """Test parsing of review snippets""" + text = '''Hi Fred, + +This is a comment from someone. + +Something else + +On some recent date, Fred wrote: +> This is why I wrote the patch +> so here it is + +Now a comment about the commit message +A little more to say + +Even more + +> diff --git a/file.c b/file.c +> Some more code +> Code line 2 +> Code line 3 +> Code line 4 +> Code line 5 +> Code line 6 +> Code line 7 +> Code line 8 +> Code line 9 + +And another comment + +> @@ -153,8 +143,13 @@ def CheckPatch(fname, show_types=False): +> further down on the file +> and more code +> +Addition here +> +Another addition here +> codey +> more codey + +and another thing in same file + +> @@ -253,8 +243,13 @@ +> with no function context + +one more thing + +> diff --git a/tools/patman/main.py b/tools/patman/main.py +> +line of code +now a very long comment in a different file +line2 +line3 +line4 +line5 +line6 +line7 +line8 +''' + pstrm = PatchStream.process_text(text, True) + self.assertEqual([], pstrm.commit.warn) + + # We expect to the filename and up to 5 lines of code context before + # each comment. The 'On xxx wrote:' bit should be removed. + self.assertEqual( + [['Hi Fred,', + 'This is a comment from someone.', + 'Something else'], + ['> This is why I wrote the patch', + '> so here it is', + 'Now a comment about the commit message', + 'A little more to say', 'Even more'], + ['> File: file.c', '> Code line 5', '> Code line 6', + '> Code line 7', '> Code line 8', '> Code line 9', + 'And another comment'], + ['> File: file.c', + '> Line: 153 / 143: def CheckPatch(fname, show_types=False):', + '> and more code', '> +Addition here', '> +Another addition here', + '> codey', '> more codey', 'and another thing in same file'], + ['> File: file.c', '> Line: 253 / 243', + '> with no function context', 'one more thing'], + ['> File: tools/patman/main.py', '> +line of code', + 'now a very long comment in a different file', + 'line2', 'line3', 'line4', 'line5', 'line6', 'line7', 'line8']], + pstrm.snippets) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index a4bf08b7d8..c165bc1825 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -4,11 +4,13 @@ """Handles parsing a stream of commits/emails from 'git log' or other source""" +import collections import datetime import io import math import os import re +import queue import shutil import tempfile @@ -51,6 +53,12 @@ RE_SPACE_BEFORE_TAB = re.compile('^[+].* \t') # Match indented lines for changes RE_LEADING_WHITESPACE = re.compile(r'^\s') +# Detect a 'diff' line +RE_DIFF = re.compile(r'^>.*diff --git a/(.*) b/(.*)$') + +# Detect a context line, like '> @@ -153,8 +153,13 @@ CheckPatch +RE_LINE = re.compile(r'>.*@@ \-(\d+),\d+ \+(\d+),\d+ @@ *(.*)') + # States we can be in - can we use range() and still have comments? STATE_MSG_HEADER = 0 # Still in the message header STATE_PATCH_SUBJECT = 1 # In patch subject (first line of log for a commit) @@ -81,6 +89,14 @@ class PatchStream: self.blank_count = 0 # Number of blank lines stored up self.state = STATE_MSG_HEADER # What state are we in? self.commit = None # Current commit + self.snippets = [] # List of unquoted test blocks + self.cur_diff = None # Last 'diff' line seen (str) + self.cur_line = None # Last context (@@) line seen (str) + self.recent_diff= None # 'diff' line for current snippet (str) + self.recent_line= None # '@@' line for current snippet (str) + self.recent_quoted = collections.deque([], 5) + self.recent_unquoted = queue.Queue() + self.was_quoted = None @staticmethod def process_text(text, is_comment=False): @@ -176,6 +192,10 @@ class PatchStream: self.skip_blank = True self.section = [] + self.cur_diff = None + self.recent_diff = None + self.recent_line = None + def _parse_version(self, value, line): """Parse a version from a *-changes tag @@ -209,6 +229,47 @@ class PatchStream: self.commit.AddChange(self.change_version, change) self.change_lines = [] + def _finalise_snippet(self): + """Finish off a snippet and add it to the list + + This is called when we get to the end of a snippet, i.e. the we enter + the next block of quoted text: + + This is a comment from someone. + + Something else + + > Now we have some code <----- end of snippet + > more code + + Now a comment about the above code + + This adds the snippet to our list + """ + quoted_lines = [] + while self.recent_quoted: + quoted_lines.append(self.recent_quoted.popleft()) + unquoted_lines = [] + valid = False + while not self.recent_unquoted.empty(): + text = self.recent_unquoted.get() + if not (text.startswith('On ') and text.endswith('wrote:')): + unquoted_lines.append(text) + if text: + valid = True + if valid: + lines = [] + if self.recent_diff: + lines.append('> File: %s' % self.recent_diff) + if self.recent_line: + out = '> Line: %s / %s' % self.recent_line[:2] + if self.recent_line[2]: + out += ': %s' % self.recent_line[2] + lines.append(out) + lines += quoted_lines + unquoted_lines + if lines: + self.snippets.append(lines) + def process_line(self, line): """Process a single line of a patch file or commit log @@ -254,6 +315,8 @@ class PatchStream: cover_match = RE_COVER.match(line) signoff_match = RE_SIGNOFF.match(line) leading_whitespace_match = RE_LEADING_WHITESPACE.match(line) + diff_match = RE_DIFF.match(line) + line_match = RE_LINE.match(line) tag_match = None if self.state == STATE_PATCH_HEADER: tag_match = RE_TAG.match(line) @@ -443,6 +506,27 @@ class PatchStream: out = [line] self.linenum += 1 self.skip_blank = False + + if diff_match: + self.cur_diff = diff_match.group(1) + + # If this is quoted, keep recent lines + if not diff_match and self.linenum > 1 and line: + if line.startswith('>'): + if not self.was_quoted: + self._finalise_snippet() + self.recent_line = None + if not line_match: + self.recent_quoted.append(line) + self.was_quoted = True + self.recent_diff = self.cur_diff + else: + self.recent_unquoted.put(line) + self.was_quoted = False + + if line_match: + self.recent_line = line_match.groups() + if self.state == STATE_DIFFS: pass @@ -466,6 +550,7 @@ class PatchStream: def finalise(self): """Close out processing of this patch stream""" + self._finalise_snippet() self._finalise_change() self._close_commit() if self.lines_after_test: From dc4b2a9770b5b932cd6d98c33ebff6dc46de6849 Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Thu, 29 Oct 2020 21:46:38 -0600 Subject: [PATCH 44/44] patman: Support listing comments from patchwork While reviewing feedback it is helpful to see the review comments on the command line to check that each has been addressed. Add an option to support that. Update the workflow documentation to describe the new features. Signed-off-by: Simon Glass --- tools/patman/README | 36 +++++-- tools/patman/control.py | 11 ++- tools/patman/func_test.py | 190 +++++++++++++++++++++++++++++++++--- tools/patman/main.py | 5 +- tools/patman/patchstream.py | 7 +- tools/patman/status.py | 59 +++++++++-- 6 files changed, 270 insertions(+), 38 deletions(-) diff --git a/tools/patman/README b/tools/patman/README index 15da6dc33c..49b73590cf 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -14,6 +14,7 @@ This tool is a Python script which: It also has some Patchwork features: - shows review tags from Patchwork so you can update your local patches - pulls these down into a new branch on request +- lists comments received on a series It is intended to automate patch creation and make it a less error-prone process. It is useful for U-Boot and Linux work so far, @@ -400,6 +401,8 @@ end. You can check that this worked with: which should show that there are no new responses compared to this new branch. +There is also a -C option to list the comments received for each patch. + Example Work Flow ================= @@ -484,17 +487,33 @@ people on the list don't see your secret info. Of course patches often attract comments and you need to make some updates. Let's say one person sent comments and you get an Acked-by: on one patch. Also, the patch on the list that you were waiting for has been merged, -so you can drop your wip commit. So you resync with upstream: +so you can drop your wip commit. + +Take a look on patchwork and find out the URL of the series. This will be +something like http://patchwork.ozlabs.org/project/uboot/list/?series=187331 +Add this to a tag in your top commit: + + Series-link: http://patchwork.ozlabs.org/project/uboot/list/?series=187331 + +You can use then patman to collect the Acked-by tag to the correct commit, +creating a new 'version 2' branch for us-cmd: + + patman status -d us-cmd2 + git checkout us-cmd2 + +You can look at the comments in Patchwork or with: + + patman status -C + +Then you can resync with upstream: git fetch origin (or whatever upstream is called) git rebase origin/master -and use git rebase -i to edit the commits, dropping the wip one. You add -the ack tag to one commit: +and use git rebase -i to edit the commits, dropping the wip one. - Acked-by: Heiko Schocher - -update the Series-cc: in the top commit: +Then update the Series-cc: in the top commit to add the person who reviewed +the v1 series: Series-cc: bfin, marex, Heiko Schocher @@ -533,7 +552,9 @@ so to send them: and it will create and send the version 2 series. -General points: + +General points +============== 1. When you change back to the us-cmd branch days or weeks later all your information is still there, safely stored in the commits. You don't need @@ -613,3 +634,4 @@ a bad thing. Simon Glass v1, v2, 19-Oct-11 revised v3 24-Nov-11 +revised v4 Independence Day 2020, with Patchwork integration diff --git a/tools/patman/control.py b/tools/patman/control.py index 6ac258d41d..f4a6ca145d 100644 --- a/tools/patman/control.py +++ b/tools/patman/control.py @@ -176,11 +176,13 @@ def send(args): args.limit, args.dry_run, args.in_reply_to, args.thread, args.smtp_server) -def patchwork_status(branch, count, start, end, dest_branch, force): +def patchwork_status(branch, count, start, end, dest_branch, force, + show_comments): """Check the status of patches in patchwork This finds the series in patchwork using the Series-link tag, checks for new - review tags, displays then and creates a new branch with the review tags. + comments and review tags, displays then and creates a new branch with the + review tags. Args: branch (str): Branch to create patches from (None = current) @@ -192,6 +194,8 @@ def patchwork_status(branch, count, start, end, dest_branch, force): dest_branch (str): Name of new branch to create with the updated tags (None to not create a branch) force (bool): With dest_branch, force overwriting an existing branch + show_comments (bool): True to display snippets from the comments + provided by reviewers Raises: ValueError: if the branch has no Series-link value @@ -223,4 +227,5 @@ def patchwork_status(branch, count, start, end, dest_branch, force): # Import this here to avoid failing on other commands if the dependencies # are not present from patman import status - status.check_patchwork_status(series, found[0], branch, dest_branch, force) + status.check_patchwork_status(series, found[0], branch, dest_branch, force, + show_comments) diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index bbee4b77d6..e2adf32c73 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -857,15 +857,16 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.patches = [patch1, patch2] count = 2 new_rtag_list = [None] * count + review_list = [None, None] # Check that the tags are picked up on the first patch - status.find_new_responses(new_rtag_list, 0, commit1, patch1, - self._fake_patchwork2) + status.find_new_responses(new_rtag_list, review_list, 0, commit1, + patch1, self._fake_patchwork2) self.assertEqual(new_rtag_list[0], {'Reviewed-by': {self.joe}}) # Now the second patch - status.find_new_responses(new_rtag_list, 1, commit2, patch2, - self._fake_patchwork2) + status.find_new_responses(new_rtag_list, review_list, 1, commit2, + patch2, self._fake_patchwork2) self.assertEqual(new_rtag_list[1], { 'Reviewed-by': {self.mary, self.fred}, 'Tested-by': {self.leb}}) @@ -874,16 +875,16 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c # 'new' tags when scanning comments new_rtag_list = [None] * count commit1.rtags = {'Reviewed-by': {self.joe}} - status.find_new_responses(new_rtag_list, 0, commit1, patch1, - self._fake_patchwork2) + status.find_new_responses(new_rtag_list, review_list, 0, commit1, + patch1, self._fake_patchwork2) self.assertEqual(new_rtag_list[0], {}) # For the second commit, add Ed and Fred, so only Mary should be left commit2.rtags = { 'Tested-by': {self.leb}, 'Reviewed-by': {self.fred}} - status.find_new_responses(new_rtag_list, 1, commit2, patch2, - self._fake_patchwork2) + status.find_new_responses(new_rtag_list, review_list, 1, commit2, + patch2, self._fake_patchwork2) self.assertEqual(new_rtag_list[1], {'Reviewed-by': {self.mary}}) # Check that the output patches expectations: @@ -898,7 +899,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c series = Series() series.commits = [commit1, commit2] terminal.SetPrintTestMode() - status.check_patchwork_status(series, '1234', None, None, False, + status.check_patchwork_status(series, '1234', None, None, False, False, self._fake_patchwork2) lines = iter(terminal.GetPrintTestLines()) col = terminal.Color() @@ -913,18 +914,18 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual(terminal.PrintLine(' 2 Subject 2', col.BLUE), next(lines)) - self.assertEqual( - terminal.PrintLine(' Tested-by: ', col.GREEN, newline=False, - bright=False), - next(lines)) - self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False), - next(lines)) self.assertEqual( terminal.PrintLine(' Reviewed-by: ', col.GREEN, newline=False, bright=False), next(lines)) self.assertEqual(terminal.PrintLine(self.fred, col.WHITE, bright=False), next(lines)) + self.assertEqual( + terminal.PrintLine(' Tested-by: ', col.GREEN, newline=False, + bright=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.leb, col.WHITE, bright=False), + next(lines)) self.assertEqual( terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False), next(lines)) @@ -1010,7 +1011,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c terminal.SetPrintTestMode() status.check_patchwork_status(series, '1234', branch, dest_branch, - False, self._fake_patchwork3, repo) + False, False, self._fake_patchwork3, repo) lines = terminal.GetPrintTestLines() self.assertEqual(12, len(lines)) self.assertEqual( @@ -1044,6 +1045,7 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual('Reviewed-by: %s' % self.mary, next(lines)) self.assertEqual('Tested-by: %s' % self.leb, next(lines)) + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') def testParseSnippets(self): """Test parsing of review snippets""" text = '''Hi Fred, @@ -1126,3 +1128,159 @@ line8 'now a very long comment in a different file', 'line2', 'line3', 'line4', 'line5', 'line6', 'line7', 'line8']], pstrm.snippets) + + @unittest.skipIf(not HAVE_PYGIT2, 'Missing python3-pygit2') + def testReviewSnippets(self): + """Test showing of review snippets""" + def _to_submitter(who): + m_who = re.match('(.*) <(.*)>', who) + return { + 'name': m_who.group(1), + 'email': m_who.group(2) + } + + commit1 = Commit('abcd') + commit1.subject = 'Subject 1' + commit2 = Commit('ef12') + commit2.subject = 'Subject 2' + + patch1 = status.Patch('1') + patch1.parse_subject('[1/2] Subject 1') + patch1.name = patch1.raw_subject + patch1.content = 'This is my patch content' + comment1a = {'submitter': _to_submitter(self.joe), + 'content': '''Hi Fred, + +On some date Fred wrote: + +> diff --git a/file.c b/file.c +> Some code +> and more code + +Here is my comment above the above... + + +Reviewed-by: %s +''' % self.joe} + + patch1.comments = [comment1a] + + patch2 = status.Patch('2') + patch2.parse_subject('[2/2] Subject 2') + patch2.name = patch2.raw_subject + patch2.content = 'Some other patch content' + comment2a = { + 'content': 'Reviewed-by: %s\nTested-by: %s\n' % + (self.mary, self.leb)} + comment2b = {'submitter': _to_submitter(self.fred), + 'content': '''Hi Fred, + +On some date Fred wrote: + +> diff --git a/tools/patman/commit.py b/tools/patman/commit.py +> @@ -41,6 +41,9 @@ class Commit: +> self.rtags = collections.defaultdict(set) +> self.warn = [] +> +> + def __str__(self): +> + return self.subject +> + +> def AddChange(self, version, info): +> """Add a new change line to the change list for a version. +> +A comment + +Reviewed-by: %s +''' % self.fred} + patch2.comments = [comment2a, comment2b] + + # This test works by setting up commits and patch for use by the fake + # Rest API function _fake_patchwork2(). It calls various functions in + # the status module after setting up tags in the commits, checking that + # things behaves as expected + self.commits = [commit1, commit2] + self.patches = [patch1, patch2] + + # Check that the output patches expectations: + # 1 Subject 1 + # Reviewed-by: Joe Bloggs + # 2 Subject 2 + # Tested-by: Lord Edmund Blackaddër + # Reviewed-by: Fred Bloggs + # + Reviewed-by: Mary Bloggs + # 1 new response available in patchwork + + series = Series() + series.commits = [commit1, commit2] + terminal.SetPrintTestMode() + status.check_patchwork_status(series, '1234', None, None, False, True, + self._fake_patchwork2) + lines = iter(terminal.GetPrintTestLines()) + col = terminal.Color() + self.assertEqual(terminal.PrintLine(' 1 Subject 1', col.BLUE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.joe, col.WHITE), next(lines)) + + self.assertEqual(terminal.PrintLine('Review: %s' % self.joe, col.RED), + next(lines)) + self.assertEqual(terminal.PrintLine(' Hi Fred,', None), next(lines)) + self.assertEqual(terminal.PrintLine('', None), next(lines)) + self.assertEqual(terminal.PrintLine(' > File: file.c', col.MAGENTA), + next(lines)) + self.assertEqual(terminal.PrintLine(' > Some code', col.MAGENTA), + next(lines)) + self.assertEqual(terminal.PrintLine(' > and more code', col.MAGENTA), + next(lines)) + self.assertEqual(terminal.PrintLine( + ' Here is my comment above the above...', None), next(lines)) + self.assertEqual(terminal.PrintLine('', None), next(lines)) + + self.assertEqual(terminal.PrintLine(' 2 Subject 2', col.BLUE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.fred, col.WHITE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' + Reviewed-by: ', col.GREEN, newline=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.mary, col.WHITE), + next(lines)) + self.assertEqual( + terminal.PrintLine(' + Tested-by: ', col.GREEN, newline=False), + next(lines)) + self.assertEqual(terminal.PrintLine(self.leb, col.WHITE), + next(lines)) + + self.assertEqual(terminal.PrintLine('Review: %s' % self.fred, col.RED), + next(lines)) + self.assertEqual(terminal.PrintLine(' Hi Fred,', None), next(lines)) + self.assertEqual(terminal.PrintLine('', None), next(lines)) + self.assertEqual(terminal.PrintLine( + ' > File: tools/patman/commit.py', col.MAGENTA), next(lines)) + self.assertEqual(terminal.PrintLine( + ' > Line: 41 / 41: class Commit:', col.MAGENTA), next(lines)) + self.assertEqual(terminal.PrintLine( + ' > + return self.subject', col.MAGENTA), next(lines)) + self.assertEqual(terminal.PrintLine( + ' > +', col.MAGENTA), next(lines)) + self.assertEqual( + terminal.PrintLine(' > def AddChange(self, version, info):', + col.MAGENTA), + next(lines)) + self.assertEqual(terminal.PrintLine( + ' > """Add a new change line to the change list for a version.', + col.MAGENTA), next(lines)) + self.assertEqual(terminal.PrintLine( + ' >', col.MAGENTA), next(lines)) + self.assertEqual(terminal.PrintLine( + ' A comment', None), next(lines)) + self.assertEqual(terminal.PrintLine('', None), next(lines)) + + self.assertEqual(terminal.PrintLine( + '4 new responses available in patchwork (use -d to write them to a new branch)', + None), next(lines)) diff --git a/tools/patman/main.py b/tools/patman/main.py index 8f139a6e3b..c7f425522b 100755 --- a/tools/patman/main.py +++ b/tools/patman/main.py @@ -92,6 +92,8 @@ AddCommonArgs(test_parser) status = subparsers.add_parser('status', help='Check status of patches in patchwork') +status.add_argument('-C', '--show-comments', action='store_true', + help='Show comments from each patch') status.add_argument('-d', '--dest-branch', type=str, help='Name of branch to create with collected responses') status.add_argument('-f', '--force', action='store_true', @@ -171,7 +173,8 @@ elif args.cmd == 'status': ret_code = 0 try: control.patchwork_status(args.branch, args.count, args.start, args.end, - args.dest_branch, args.force) + args.dest_branch, args.force, + args.show_comments) except Exception as e: terminal.Print('patman: %s: %s' % (type(e).__name__, e), colour=terminal.Color.RED) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index c165bc1825..772e4b5661 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -89,11 +89,12 @@ class PatchStream: self.blank_count = 0 # Number of blank lines stored up self.state = STATE_MSG_HEADER # What state are we in? self.commit = None # Current commit - self.snippets = [] # List of unquoted test blocks + # List of unquoted test blocks, each a list of str lines + self.snippets = [] self.cur_diff = None # Last 'diff' line seen (str) self.cur_line = None # Last context (@@) line seen (str) - self.recent_diff= None # 'diff' line for current snippet (str) - self.recent_line= None # '@@' line for current snippet (str) + self.recent_diff = None # 'diff' line for current snippet (str) + self.recent_line = None # '@@' line for current snippet (str) self.recent_quoted = collections.deque([], 5) self.recent_unquoted = queue.Queue() self.was_quoted = None diff --git a/tools/patman/status.py b/tools/patman/status.py index f3a654160e..a369d655c5 100644 --- a/tools/patman/status.py +++ b/tools/patman/status.py @@ -3,8 +3,9 @@ # Copyright 2020 Google LLC # """Talks to the patchwork service to figure out what patches have been reviewed -and commented on. Allows creation of a new branch based on the old but with the -review tags collected from patchwork. +and commented on. Provides a way to display review tags and comments. +Allows creation of a new branch based on the old but with the review tags +collected from patchwork. """ import collections @@ -124,6 +125,25 @@ class Patch(dict): self.seq = 1 self.count = 1 + +class Review: + """Represents a single review email collected in Patchwork + + Patches can attract multiple reviews. Each consists of an author/date and + a variable number of 'snippets', which are groups of quoted and unquoted + text. + """ + def __init__(self, meta, snippets): + """Create new Review object + + Args: + meta (str): Text containing review author and date + snippets (list): List of snippets in th review, each a list of text + lines + """ + self.meta = ' : '.join([line for line in meta.splitlines() if line]) + self.snippets = snippets + def compare_with_series(series, patches): """Compare a list of patches with a series it came from @@ -241,7 +261,8 @@ def collect_patches(series, series_id, rest_api=call_rest_api): patches = sorted(patches, key=lambda x: x.seq) return patches -def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api): +def find_new_responses(new_rtag_list, review_list, seq, cmt, patch, + rest_api=call_rest_api): """Find new rtags collected by patchwork that we don't know about This is designed to be run in parallel, once for each commit/patch @@ -252,6 +273,9 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api): key: Response tag (e.g. 'Reviewed-by') value: Set of people who gave that response, each a name/email string + review_list (list): New reviews are written to review_list[seq] + list, each a + List of reviews for the patch, each a Review seq (int): Position in new_rtag_list to update cmt (Commit): Commit object for this commit patch (Patch): Corresponding Patch object for this patch @@ -271,8 +295,13 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api): data = rest_api('patches/%s/comments/' % patch.id) + reviews = [] for comment in data: pstrm = PatchStream.process_text(comment['content'], True) + if pstrm.snippets: + submitter = comment['submitter'] + person = '%s <%s>' % (submitter['name'], submitter['email']) + reviews.append(Review(person, pstrm.snippets)) for response, people in pstrm.commit.rtags.items(): rtags[response].update(people) @@ -286,6 +315,7 @@ def find_new_responses(new_rtag_list, seq, cmt, patch, rest_api=call_rest_api): if is_new: new_rtags[tag].add(who) new_rtag_list[seq] = new_rtags + review_list[seq] = reviews def show_responses(rtags, indent, is_new): """Show rtags collected @@ -302,8 +332,9 @@ def show_responses(rtags, indent, is_new): """ col = terminal.Color() count = 0 - for tag, people in rtags.items(): - for who in people: + for tag in sorted(rtags.keys()): + people = rtags[tag] + for who in sorted(people): terminal.Print(indent + '%s %s: ' % ('+' if is_new else ' ', tag), newline=False, colour=col.GREEN, bright=is_new) terminal.Print(who, colour=col.WHITE, bright=is_new) @@ -376,7 +407,8 @@ def create_branch(series, new_rtag_list, branch, dest_branch, overwrite, return num_added def check_patchwork_status(series, series_id, branch, dest_branch, force, - rest_api=call_rest_api, test_repo=None): + show_comments, rest_api=call_rest_api, + test_repo=None): """Check the status of a series on Patchwork This finds review tags and comments for a series in Patchwork, displaying @@ -388,6 +420,7 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force, branch (str): Existing branch to update, or None dest_branch (str): Name of new branch to create, or None force (bool): True to force overwriting dest_branch if it exists + show_comments (bool): True to show the comments on each patch rest_api (function): API function to call to access Patchwork, for testing test_repo (pygit2.Repository): Repo to use (use None unless testing) @@ -396,6 +429,7 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force, col = terminal.Color() count = len(series.commits) new_rtag_list = [None] * count + review_list = [None] * count patch_for_commit, _, warnings = compare_with_series(series, patches) for warn in warnings: @@ -405,8 +439,8 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force, with concurrent.futures.ThreadPoolExecutor(max_workers=16) as executor: futures = executor.map( - find_new_responses, repeat(new_rtag_list), range(count), - series.commits, patch_list, repeat(rest_api)) + find_new_responses, repeat(new_rtag_list), repeat(review_list), + range(count), series.commits, patch_list, repeat(rest_api)) for fresponse in futures: if fresponse: raise fresponse.exception() @@ -425,6 +459,15 @@ def check_patchwork_status(series, series_id, branch, dest_branch, force, indent = ' ' * 2 show_responses(base_rtags, indent, False) num_to_add += show_responses(new_rtags, indent, True) + if show_comments: + for review in review_list[seq]: + terminal.Print('Review: %s' % review.meta, colour=col.RED) + for snippet in review.snippets: + for line in snippet: + quoted = line.startswith('>') + terminal.Print(' %s' % line, + colour=col.MAGENTA if quoted else None) + terminal.Print() terminal.Print("%d new response%s available in patchwork%s" % (num_to_add, 's' if num_to_add != 1 else '',