Page 1 of 3

pm2ubcd script discussion

Posted: Mon Apr 01, 2013 8:03 pm
by ady
_ 2013MAY01 EDIT:
_ Changed the order of the arguments, according to the first beta. When executed with no arguments, a simple help is displayed. The arguments are increasingly optional, so it is possible to provide just the "pmagic-extracted" path and the script should work, asking confirmation from the user.
_ First beta attached to post #12, reply #11.
_ Beta2 attached to post #15
2013MAY13 EDIT:
_ Beta3 attached to post #22
2013JUN25 EDIT:
_ pm2ubcd2013JUN25 attached to post #43
2013JUL29 EDIT:
_ pm2ubcd2013JUL29 attached to post #44

Older posts:
post #43 ; post #22 and post #12 and post #15 are still relevant (read them)

______


I am seeking help here to put together a pm2ubcd.sh script to customize UBCD with an updated version of PMagic. Since PMagic releases are much more frequent than UBCD's ones, this should simplify the task for users, in a similar way as other scripts in UBCD. If it works as expected, Victor can hopefully add it to the next release of UBCD.

Here are the minimal conditions and goals:

_ The script would be located under
ubcd-extracted/ubcd/tools/linux/pm2ubcd/
, named
pm2ubcd.sh
.

_ The task of downloading and extracting the content of (Pmagic's and UBCD's) ISO images is up to the user.

_ The script should not care about the specific version or variant of PMagic. The user might want to use an older version of PMagic, or a newer one, or a different variant (i586, i686, x86_64).

_ The script should work on UBCD v5.2.1. Older versions of UBCD won't be supported by the script.

_ Similarly to other scripts in UBCD, the arguments needed by the script would be:
13_ "iso_filename=" parameter (to modify PMagic's syslinux.cfg);
21_ Path where the content of the PMagic ISO image is located (already extracted by the user);
32_ Path where the content of UBCD is located (already extracted by the user).

There could be an alternative way to the first argument. Instead of the user having to provide the "iso_filename=" argument, it could (potentially) be read from other files in "
ubcd-extracted
". For the time being, I'm inclined to request this parameter from the user.

_ The script should:

1_ Delete the prior squashfs PMagic file(s) such as

Code: Select all

ubcd-extracted/pmagic/pmodules/PMAGIC_*.SQFS
Any other (modules, scripts, personal, configuration) files are not to be touched.

2_ Move
ubcd-extracted/pmagic/boot/syslinux/*.cfg
to
ubcd-extracted/pmagic/boot/syslinux/*_<MOVE_DATE__MOVE_TIME>.cfg.old


3_ Copy (overwriting files and merging directories):

Code: Select all

pmagic-extracted/pmagic/    ==>>  ubcd-extracted/pmagic/

Code: Select all

pmagic-extracted/boot/    ==>>  ubcd-extracted/pmagic/boot/
4_ Delete unnecessary files (or exclude them from the previous "copy" step):

Code: Select all

ubcd-extracted/pmagic/boot/syslinux/*.c32
ubcd-extracted/pmagic/boot/syslinux/isohybrid
ubcd-extracted/pmagic/boot/syslinux/isolinux*.*
ubcd-extracted/pmagic/boot/syslinux/mbr.bin
ubcd-extracted/pmagic/boot/syslinux/memdisk
5_ Using "sed" and other "auxiliary tools" in a similar way as used in other scripts already included in UBCD, the new pm2ubcd.sh script should edit
ubcd-extracted/pmagic/boot/syslinux/*.cfg
as follows:

5.1_

Code: Select all

F<digit> *message*.txt
    ==>>  F<digit> /pmagic/boot/syslinux/message*.txt
5.2_

Code: Select all

 initrd=/pmagic/initrd.img 
   ==>>   initrd=/pmagic/initrd.img iso_filename=ubcd521.iso
which in fact is dependent on the first argument of the script but I used here "
ubcd521.iso
" just for clarity.

6_ Finally, executing the script without options should offer help about the script (let's leave this condition for later discussion).

The above conditions cover i586, i686 and dual x32/x64 variants of PMagic and UBCD v5.2.x.


The following are "parts and pieces" of the above condition #5 of the pm2ubcd.sh script, in alpha stage; has NOT been tested, and it may possibly contain pseudo-code.

Code: Select all

# Change the working directory to the root directory of the extracted iso.
SCRIPTDIR=`dirname "${0}"`
cd "${SCRIPTDIR}/../../../../"

# Get all syslinux config files from /pmagic/boot/syslinux/, that need to be edited.
SYSLINUX_CONFIGFILES_PMAGIC=`find ./pmagic/boot/syslinux/ -name '*.cfg'`

# 'Point to /pmagic/boot/syslinux/ where appropriate:'
for SYSLINUX_CONFIGFILE_PMAGIC in $SYSLINUX_CONFIGFILES_PMAGIC; do
	sed 's|^F[[:digit:]] \(.*\)\message\(.*\)\.txt|^F[[:digit:]] /pmagic/boot/syslinux/message\2\.txt'
done

# NEED TO ESTABLISH ISO_FILENAME VARIABLE FROM SCRIPT FIRST ARGUMENT
#
# 'Add iso_filename parameter'
# for SYSLINUX_CONFIGFILE_PMAGIC in $SYSLINUX_CONFIGFILES_PMAGIC; do
# 	sed 's| initrd=/pmagic/initrd.img | initrd=/pmagic/initrd.img iso_filename=ubcd521.iso'
# done

I would like to request help to put together the complete script, respecting all the aforementioned conditions / steps.

Suggestions, ideas, improvements, code... are all welcome.

TIA,
Ady.

Re: pm2ubcd script discussion

Posted: Mon Apr 01, 2013 11:34 pm
by Explorer09
Here are two things that you might need to add to the script:

1. Remove "pmagic-extracted/mkgriso" script. It's useless in UBCD.

2. Edit "ubcd-extracted/ubcd/menus/syslinux/main.cfg" to update the menu entry.

Replace:

Code: Select all

MENU LABEL Parted Magic 2013_01_29_i586 - Press F1 for more information
TEXT HELP
 ......
ENDTEXT
CONFIG /pmagic/boot/syslinux/syslinux.cfg /boot/syslinux/
With:

Code: Select all

MENU LABEL Parted Magic <new-version> - Press F1 for more information
TEXT HELP
 ......
ENDTEXT
CONFIG /pmagic/boot/syslinux/syslinux.cfg /boot/syslinux/

Re: pm2ubcd script discussion

Posted: Tue Apr 02, 2013 3:17 am
by ady
Explorer09 wrote:Here are two things that you might need to add to the script:

1. Remove "pmagic-extracted/mkgriso" script. It's useless in UBCD.
Thank you for the feedback.

The new pm2ubcd script should not copy the root directory of "
pmagic-extracted/
", so mkgriso is not copied/moved to "
ubcd-extracted/
". The conditions I mentioned above are "minimal", so one optional additional final step could be to delete "
pmagic-extracted
". Other scripts in UBCD don't delete "anything", so for now I am following the same principle.
2. Edit "ubcd-extracted/ubcd/menus/syslinux/main.cfg" to update the menu entry.

Code: Select all

MENU LABEL Parted Magic <new-version> - Press F1 for more information
Yes, I am thinking about this too. Again, the conditions I mentioned above are "minimal". Changing the version in main.cfg is not critical regarding the functionality, but indeed it should be considered. One alternative possibility would be for Victor not to include the version number of PMagic in main.cfg.

In case the version "must" be corrected in main.cfg, it would be taken from the SQFS file name.

The correct replacement of the version number in main.cfg might be "trickier" than the other "sed" substitutions. Any other part of the new pm2ubcd.sh script would act on
ubcd-extracted/pmagic/
or down the path tree, but main.cfg is another story (and it might contain other customizations too).

The "easier" way would be if Victor would consider not specifying the version of PMagic in main.cfg, specially since the "version" (date) is already included in
ubcd-extracted/pmagic/boot/syslinux/syslinux.cfg
by default.

Please keep the suggestions, corrections, (and even code 8) ) coming.

Re: pm2ubcd script discussion

Posted: Tue Apr 02, 2013 4:49 am
by Explorer09
ady wrote: The new pm2ubcd script should not copy the root directory of "
pmagic-extracted/
", so mkgriso is not copied/moved to "
ubcd-extracted/
". The conditions I mentioned above are "minimal", so one optional additional final step could be to delete "
pmagic-extracted
". Other scripts in UBCD don't delete "anything", so for now I am following the same principle.
You're right, don't delete anything from the source directory. I meant that too, but I mistakenly say to 'delete' instead of to 'ignore' the file.
ady wrote:In case the version "must" be corrected in main.cfg, it would be taken from the SQFS file name.
Yes. I meant that too.
ady wrote:Please keep the suggestions, corrections, (and even code) coming.
Here you are. Code is untested, so use at your own risk.

Code: Select all

#!/bin/sed
s|^UI menu\.c32|UI /boot/syslinux/menu.c32|
s|^F\([1-9]\) /boot/syslinux/message\([0-9]*\)\.txt|F\1 /pmagic/boot/syslinux/message\2.txt|g

Code: Select all

#!/bin/sed
# if the filename is hard-coded in the sed script
s|^\(APPEND /pmagic/bzImage initrd=/pmagic/initrd\.img [^\r\n]*\)$|\1 iso_filename=ubcd521.iso|g

Code: Select all

#!/bin/bash
# if the filename is stored in a variable in a shell script
ISO_FILENAME=ubcd521.iso
sed -e "s|^\(APPEND /pmagic/bzImage initrd=/pmagic/initrd\.img [^\r\n]*\)\$|\1 iso_filename=${ISO_FILENAME}|g"

Code: Select all

#!/bin/sed
# chntpw, Replace:
# APPEND /boot/chntpw/vmlinuz rw vga=1 loglevel=0 initrd=/boot/chntpw/initrd.cgz,/boot/chntpw/scsi.cgz
# with:
# APPEND /pmagic/boot/chntpw/vmlinuz rw vga=1 loglevel=0 initrd=/pmagic/boot/chntpw/initrd.cgz,/pmagic/boot/chntpw/scsi.cgz
s|^APPEND /boot/chntpw/vmlinuz \([^\r\n]*\) initrd=/boot/chntpw/initrd\.cgz,/boot/chntpw/scsi\.cgz([^\r\n]*)|APPEND /pmagic/boot/chntpw/vmlinuz \1 initrd=/pmagic/boot/chntpw/initrd.cgz,/pmagic/boot/chntpw/scsi.cgz\2|

# ipxe, replace:
# KERNEL /boot/ipxe/ipxe.krn
# with:
# KERNEL /pmagic/boot/ipxe/ipxe.krn
s|^KERNEL /boot/ipxe/ipxe\.krn|KERNEL /pmagic/boot/ipxe/ipxe.krn|

# plpbt and memtest, replace:
# LINUX /boot/plpbt/plpbt.bin
# with:
# LINUX /pmagic/boot/plpbt/plpbt.bin
s|^LINUX /boot/\([\./a-z]*\)|LINUX /pmagic/boot/\1|g

# hdt, sgd, sgd2, and mhdd, replace:
# LINUX /boot/syslinux/memdisk
# APPEND iso initrd=/boot/sgd/sgd2.gz
# with:
# LINUX memdisk
# APPEND iso initrd=/pmagic/boot/sgd/sgd2.gz
s|^LINUX /boot/syslinux/memdisk\r\nAPPEND \([^\r\n]*\)initrd=/boot/\([\./a-z]*\)|LINUX memdisk\r\nAPPEND \1initrd=/pmagic/boot/\2|g

# reboot.c32, replace:
# COM32 /boot/syslinux/reboot.c32
# with:
# COM32 reboot.c32
s|^COM32 /boot/syslinux/reboot\.c32|COM32 reboot.c32|

Re: pm2ubcd script discussion

Posted: Tue Apr 02, 2013 7:35 am
by ady
Explorer09 wrote:You're right, don't delete anything from the source directory. I meant that too, but I mistakenly say to 'delete' instead of to 'ignore' the file.
The new pm2ubcd.sh script would, at least as default option, copy (not move, so the original file is preserved) from
pmagic-extracted/pmagic/
and from
pmagic-extracted/boot/
, not from
pmagic-extracted/
, so mkgriso and other files and directories are ignored (at least for now).
ady wrote:In case the version "must" be corrected in main.cfg, it would be taken from the SQFS file name.
Yes. I meant that too.
Hopefully Victor removes the mention of a specific PMagic version from main.cfg, which would make it easier and more generic. As said, the version is already included in PMagic's syslinux.cfg itself.

Code: Select all

#!/bin/sed
s|^UI menu\.c32|UI /boot/syslinux/menu.c32|
s|^F\([1-9]\) /boot/syslinux/message\([0-9]*\)\.txt|F\1 /pmagic/boot/syslinux/message\2.txt|g
Thank you. Posting code is very helpful.

The UI substitution is unnecessary.
Regarding the Fn substitution, what's wrong with the code I posted before?

Code: Select all

#!/bin/sed
# if the filename is hard-coded in the sed script
s|^\(APPEND /pmagic/bzImage initrd=/pmagic/initrd\.img [^\r\n]*\)$|\1 iso_filename=ubcd521.iso|g
No, the desired "iso_filename" is an argument of the script, to be provided by the user from the command line (see conditions in OP).

Also, the specific kernel MUST not be part of the substitution, so the script can deal with any PMagic variant ("bzImage*"). My suggested code (in the OP) regarding the addition of "iso_filename" is not finished because it doesn't consider (yet) the command line argument input. It just searches " initrd=/pmagic/initrd.img " (with initial and ending space characters), and adds to it the "iso_filename" kernel argument.

I'd rather not search for entire lines (starting from "append" in this particular case) as in your suggested code, because searching for the minimal necessary expression covers more potential future changes in PMagic's syslinux.cfg (which are not under our control).

Code: Select all

#!/bin/bash
# if the filename is stored in a variable in a shell script
ISO_FILENAME=ubcd521.iso
sed -e "s|^\(APPEND /pmagic/bzImage initrd=/pmagic/initrd\.img [^\r\n]*\)\$|\1 iso_filename=${ISO_FILENAME}|g"
See previous comment.

Code: Select all

#!/bin/sed
# chntpw, Replace:
# APPEND /boot/chntpw/vmlinuz rw vga=1 loglevel=0 initrd=/boot/chntpw/initrd.cgz,/boot/chntpw/scsi.cgz
# with:
# APPEND /pmagic/boot/chntpw/vmlinuz rw vga=1 loglevel=0 initrd=/pmagic/boot/chntpw/initrd.cgz,/pmagic/boot/chntpw/scsi.cgz
s|^APPEND /boot/chntpw/vmlinuz \([^\r\n]*\) initrd=/boot/chntpw/initrd\.cgz,/boot/chntpw/scsi\.cgz([^\r\n]*)|APPEND /pmagic/boot/chntpw/vmlinuz \1 initrd=/pmagic/boot/chntpw/initrd.cgz,/pmagic/boot/chntpw/scsi.cgz\2|
Isn't enough to search for "
/boot/chntpw/
" and change it to "
/pmagic/boot/chntpw/
"?

Code: Select all

# ipxe, replace:
# KERNEL /boot/ipxe/ipxe.krn
# with:
# KERNEL /pmagic/boot/ipxe/ipxe.krn
s|^KERNEL /boot/ipxe/ipxe\.krn|KERNEL /pmagic/boot/ipxe/ipxe.krn|

# plpbt and memtest, replace:
# LINUX /boot/plpbt/plpbt.bin
# with:
# LINUX /pmagic/boot/plpbt/plpbt.bin
s|^LINUX /boot/\([\./a-z]*\)|LINUX /pmagic/boot/\1|g
For plop, I'd rather search for "^linux *plpbt.bin" and replace it with the corrected path.

For the others, isn't enough to search for "
^kernel /boot/
" (possible case insensitive) and replace it with "
^kernel /pmagic/boot/
"?

Code: Select all

# hdt, sgd, sgd2, and mhdd, replace:
# LINUX /boot/syslinux/memdisk
# APPEND iso initrd=/boot/sgd/sgd2.gz
# with:
# LINUX memdisk
# APPEND iso initrd=/pmagic/boot/sgd/sgd2.gz
s|^LINUX /boot/syslinux/memdisk\r\nAPPEND \([^\r\n]*\)initrd=/boot/\([\./a-z]*\)|LINUX memdisk\r\nAPPEND \1initrd=/pmagic/boot/\2|g
For memdisk, not necessary. For others, I'd rather search for "
initrd=/boot/
" and replace it with "
initrd=/pmagic/boot/
".

Code: Select all

# reboot.c32, replace:
# COM32 /boot/syslinux/reboot.c32
# with:
# COM32 reboot.c32
s|^COM32 /boot/syslinux/reboot\.c32|COM32 reboot.c32|
Changes for *.c32 files are not necessary.

Please don't hesitate to comment, add feedback, questions, code... All are very welcome and helpful. Keep them coming :D .

Re: pm2ubcd script discussion

Posted: Tue Apr 02, 2013 8:35 am
by Explorer09
My code is a really conservative example, based on the 'diff' between the old and new config files. You are free to edit on your own, without notifying me about that.

Re: pm2ubcd script discussion

Posted: Sat Apr 27, 2013 3:28 pm
by StopSpazzing
Any way to do this server side? I have now own my own VPS with some high specs, was wondering if its possible to take the UBCD and Pmagic, both latest versions, from download sources, and verify Hash, and then Merge them server side, then provide a download with Hash. I was thinking with PHP it would be possible, but would require advanced coding that's beyond my expertise.

Re: pm2ubcd script discussion

Posted: Sun Apr 28, 2013 5:23 am
by Explorer09
Hi StopSpazzing:
Making a script in server side is not our goal. Our script is for end-users who have downloaded a Pmagic and a UBCD and want to merge Pmagic to it.

Re: pm2ubcd script discussion

Posted: Sun Apr 28, 2013 10:31 am
by StopSpazzing
Explorer09 wrote:Hi StopSpazzing:
Making a script in server side is not our goal. Our script is for end-users who have downloaded a Pmagic and a UBCD and want to merge Pmagic to it.
I understand this, just curious if it was easy to do.


Does this script work at this moment?

Re: pm2ubcd script discussion

Posted: Sun Apr 28, 2013 1:34 pm
by ady
StopSpazzing wrote:Does this script work at this moment?
Yes :D (with the invaluable, behind the scenes help of Explorer09). Expect a first public beta in the next few days.

Re: pm2ubcd script discussion

Posted: Sun Apr 28, 2013 5:05 pm
by StopSpazzing
ady wrote:
StopSpazzing wrote:Does this script work at this moment?
Yes :D (with the invaluable, behind the scenes help of Explorer09). Expect a first public beta in the next few days.
Wow, I look forward to trying it out. :)

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 12:45 am
by ady
Here is the first beta.

I want to specially thank Explorer09 8) for his invaluable, behind-the-scenes help. He certainly deserves much of the credit.

The respective scripts should be saved under the following 2 directories:

Code: Select all

"ubcd-extracted/ubcd/tools/linux/pm2ubcd/*"
"ubcd-extracted/ubcd/tools/win32/pm2ubcd/*"
so the respective directories need to be created.

The attached archive contains the following:

Code: Select all

"pm2ubcd/ubcd/tools/linux/pm2ubcd/pm2ubcd.sh"
"pm2ubcd/ubcd/tools/linux/pm2ubcd/pm2ubcd.sed"
"pm2ubcd/ubcd/tools/win32/pm2ubcd/pm2ubcd.cmd"
"pm2ubcd/ubcd/tools/win32/pm2ubcd/pm2ubcd.txt"
The above last text file is included only in one directory, but it is relevant for both, the Linux-based script and the Windows-based one.

Please carefully read the text file before testing.

As usual, I recommend having back ups.

DISCLAIMER: The user (you) is the only one responsible for whatever happens or doesn't happen; don't blame anyone else.

Feedback is appreciated.

Code: Select all

FILE : pm2ubcd.tar.gz
SIZE : 6'357 bytes
MD5  : 39ec5c0becda4bb648dc24d04142a230
SHA1 : e253e73a3f1ca936da5c47798a7e09af9c324d60
[/s]
EDIT: Beta1 deleted. See post #15 for beta2.

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 5:20 am
by Explorer09
Here's my bug report: (Using pm2ubcd beta in the previous post.)

The
iso_filename=
option is not inserted to syslinux.cfg when running the script in Windows (MSYS Bash).
Because I know how this bug happens, I can provide a patch for it.
Change this line:

Code: Select all

sed -e "s| initrd=/pmagic/initrd\.img | initrd=/pmagic/initrd\.img iso_filename=${ISO_NAME} |" | \
To this:

Code: Select all

sed -e "s| initrd=[/]pmagic[/]initrd\.img | initrd=\/pmagic\/initrd\.img iso_filename=${ISO_NAME} |" | \
I know the sed command will look ugly here, but it is the way to prevent MSYS bash from treating the text as paths, doing conversions, and bringing us trouble.

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 6:03 am
by Explorer09
Double post, because this one is about an improvement.

Code: Select all

# pwd-msys prints the working directory in the Windows format, for display.
# Works with MSYS Bash only!!
pwd-msys () {
    if [ "X$OSTYPE" = "Xmsys" ]; then
        pwd -W | sed -e 's|/|\\|g';
    fi
}
This function is a wrapper for pwd that can output a Windows path. You can use it on user messages, but don't store them in variables since the Windows paths cannot be interpreted in unix shells.

By the way, for outputting PMAGIC_ROOT and UBCD_ROOT, try this:

Code: Select all

( cd $PMAGIC_ROOT && pwd-msys ; ) 
( cd $UBCD_ROOT && pwd-msys ; ) 

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 11:56 am
by ady
Thank you Explorer09.

Arrrrggggh :oops: . In the first beta, I forgot about the square brackets for "paths as text" in sed.

Regarding the suggested improvement to display Windows-style paths when it's relevant...

It should be noted that "pwd -W" should be "enough" (no real need for changing "/" into "\"). In spite of what many users think, Windows, since several years ago already, is perfectly fine with slashes "/" for paths.

In any case, the suggestion is valid. I moved (up) the check that confirms whether the paths exist, so to be able to use "pwd-msys" to display paths "as expected under each respective OS".

I limited the "slash inversion" (to "\") under Windows for the main directory paths and for subdirectories that are displayed by themselves; the rest (in relative notation) will still show slash "/". The relevant paths, specially drive letters, are correctly displayed as expected by users, so the information provided by the script should be more clear now.

FWIW, in the editions I made for this beta2, I prioritized "plain language" rather than "elegant code" (ie. "echo blank spaces", over "echo horizontal tabs"). BTW, I tried to follow the same principle in sed editions.

Beta2, attached to this post.

Code: Select all

FILE : pm2ubcd_b2.tar.gz
SIZE : 6'586 bytes
MD5  : 10c728034141070812d7692e8ef58a1c
SHA1 : cd257b803fe80f4941d723a7a785952db8ee095a
[/s]

UPDATE: beta2 deleted. See post #22 for beta3.
Feedback is appreciated.

Regards,
Ady.

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 9:22 pm
by Explorer09
I'm sorry, but no offense. I can't stand reading poorly-styled code, so I cleaned it up a little.

Here is the diff file:
https://dl.dropboxusercontent.com/u/701 ... cd_b2.diff
MD5: be63297517f46efb7d6ddbd9bd4a10c3

(You could just run "patch < pm2ubcd_b2.diff" to patch it. But make sure you back up the old script first.)

Below is an explanation of the changes:

@@ -12,11 +12,8 @@ : Remove redundant comments, and avoid double equal signs in 'test' (which is bashism).

@@ -161,9 +158,9 @@ : Make good use of command substitution (i.e. the backticks). This avoids the non-portable "echo -n" and makes the code cleaner.

@@ -176,7 +173,7 @@ : "-or" switch in 'find' is non-POSIX. Enough said.

@@ -191,7 +188,7 @@ : This guide says it is better to use an explicit variable in 'read' rather than implicit.

@@ -321,7 +318,9 @@ : Change the 'find' logic.

@@ -350,7 +349,7 @@ : "\1" in 'sed' can save you a few characters. (I tend to avoid "&" for whole regex match as it is not so portable as "\1")

Re: pm2ubcd script discussion

Posted: Wed May 01, 2013 10:46 pm
by ady
Explorer09 wrote:I'm sorry, but no offense. I can't stand reading poorly-styled code, so I cleaned it up a little.
I maintain what I said; feedback is welcome :).
@@ -12,11 +12,8 @@ : Remove redundant comments, and avoid double equal signs in 'test' (which is bashism).
I will change some of those comments, but I want to make clear that I *know* that I commented "too much" in the script, intentionally.

During the years, contributors come and go. When a change is needed and there is no knowledgeable developer around, "nothing" can be done. This type of comments (somehow repeating what is already "evident" and not really adding to the code) are not just unneeded but also unwanted by developers, but they help "common poor mortals" so to try to understand (the logic in the script) in case a change is needed in the future (or in customizations).

Regarding the double equal sign in tests, I'll change the one you pointed to and a second one too. This needs intentional error testing under different OS, just in case.
@@ -161,9 +158,9 @@ : Make good use of command substitution (i.e. the backticks). This avoids the non-portable "echo -n" and makes the code cleaner.
Hmm, I was (wrongly) convinced the "-n" was portable. OK, changed.
@@ -176,7 +173,7 @@ : "-or" switch in 'find' is non-POSIX. Enough said.
Actually, I used "-or" (instead of "-o") because of the MSYS and CYGWIN ports in UBCD. Are you sure this is the right move?
@@ -191,7 +188,7 @@ : This guide says it is better to use an explicit variable in 'read' rather than implicit.
Agreed. Changed.
@@ -321,7 +318,9 @@ : Change the 'find' logic.
I had this kind of logic before, but it was giving me troubles. Have you already tested this change?

(I also suspect that this logic should be faster, but I gave up when I found problems, so it would be nice to go back to use it, if it really works under all relevant circumstances.)

Of course, the question of POSIX is relevant here too ("-not" and "-or").
@@ -350,7 +349,7 @@ : "\1" in 'sed' can save you a few characters. (I tend to avoid "&" for whole regex match as it is not so portable as "\1")
Yes, the "\1" (and "\2"...) is used in several places in UBCD's scripts already. But this is (although, arguable) one example of prioritizing "plain language". The text to be repeated is not long, so "repeating" the text is not such a big deal (I know developers don't stand this anyway). When a "common poor mortal" reads the code as seen in the cfg file, it is easier to understand and to customize it. A user trying to understand the "\1" needs to read sed tutorials.

OTOH, the text in this particular case is not exactly "repeated" and not exactly the "same" as in the cfg file, so I might change my point of view regarding this particular line.

Re: pm2ubcd script discussion

Posted: Thu May 02, 2013 12:26 am
by Explorer09
ady wrote:During the years, contributors come and go. When a change is needed and there is no knowledgeable developer around, "nothing" can be done. This type of comments (somehow repeating what is already "evident" and not really adding to the code) are not just unneeded but also unwanted by developers, but they help "common poor mortals" so to try to understand (the logic in the script) in case a change is needed in the future (or in customizations).
Ok, you persuaded me. Before you made this comment, I didn't know about the non-developers trying to customize the code. Now I'll make improvements that focus on readability for the "common poor mortals".

Regarding your other comments:
@@ -12,11 +12,8 @@ : How about changing it to this?

Code: Select all

# pwd-msys is a function that prints the working directory in the Windows 
# format. It is used in this script for displaying user messages.
# You probably won't use it in other places. (Because Windows paths cannot be
# interpreted in Unix shells.)
# The '-w' switch in pwd works with MSYS Bash only.
@@ -161,9 +158,9 @@: By 'non-portable' I mean it's Bash-only. While it doesn't matter here I think using backticks are better.

@@ -176,7 +173,7 @@: You can keep using '-or' for easier understanding by the "mortal". I don't object that.

@@ -321,7 +318,9 @@: How about re-formatting like this?

Code: Select all

SYSLINUXDIR_FILES=`find . -type f -not \( -name 'iso*' \) \
                          -not \( -name 'memdisk' \) -not \( -name '*.bin' \) \
                          -not \( -name '*.c32' \) -not \( -name 'efi*' \)`

Re: pm2ubcd script discussion

Posted: Thu May 02, 2013 1:45 am
by ady
Explorer09 wrote:Ok, you persuaded me. Before you made this comment, I didn't know about the non-developers trying to customize the code. Now I'll make improvements that focus on readability for the "common poor mortals".
Not only customizations (which I usually try to keep in mind, but it is not the first concern). It already happened more than once that something was done for UBCD, and the original contributor is not around anymore. Until someone else with relevant skills shows up ("if" someone ever shows up), no changes to that "something" can be made. There is obviously a limit to how much something can be simplified or explained for "mortals" :), but if it can be done then I would tend to favor that.
Regarding your other comments:
@@ -12,11 +12,8 @@ : How about changing it to this?

Code: Select all

# pwd-msys is a function that prints the working directory in the Windows 
# format. It is used in this script for displaying user messages.
# You probably won't use it in other places. (Because Windows paths cannot be
# interpreted in Unix shells.)
# The '-w' switch in pwd works with MSYS Bash only.
Sounds reasonable. Note that, at least in the way I used it in beta2, pwd-msys prints "Windows-like" paths under Windows, and Linux-like (not Windows-like) paths under Linux.
@@ -161,9 +158,9 @@: By 'non-portable' I mean it's Bash-only. While it doesn't matter here I think using backticks are better.
Yes, I understood it as Bash-only. I (wrongly) thought that "echo -n" _was_ portable. I just re-checked it: Ubuntu's Dash includes it too, but it is not portable (no echo's options are). Using backticks _is_ better.
@@ -176,7 +173,7 @@: You can keep using '-or' for easier understanding by the "mortal". I don't object that.
Understanding "-or" better than understanding "-o" is not a factor in this case, IMHO.

"Understanding" is nice; "correct functionality" is better. So the question is, which operators (POSIX ones or not) are better for this context where it has to work under Linux and Windows.

Potential scenario: several years from now someone suggests updating the auxiliary unxutils programs in UBCD because of some new need (or bug found, or whatever). Some MSYS tools are replaced by their CYGWIN counterparts, and the other way around (again, for some new needs, or customizations, or whichever other reason).

Giving this potential scenario, which operators (POSIX ones or not) would be more suited? I don't care if this script is usable "as-is" under, say, Solaris, or under any OS other than Linux and Windows.

So, I'm really asking. If "-o" is better suited for UBCD, let's use that. If "-or" is, then let's use it.
@@ -321,7 +318,9 @@: How about re-formatting like this?

Code: Select all

SYSLINUXDIR_FILES=`find . -type f -not \( -name 'iso*' \) \
                          -not \( -name 'memdisk' \) -not \( -name '*.bin' \) \
                          -not \( -name '*.c32' \) -not \( -name 'efi*' \)`
Sure, no problem. But I am still curious about your suggestion to change it to use "ORs" instead of multiple "NOTs" (specially since I originally intended to use "or" too). What made you think about changing it? And, have you tested it (since I originally had problems, thus I changed it to multiple "NOTs").

Re: pm2ubcd script discussion

Posted: Thu May 02, 2013 6:55 pm
by Explorer09
ady wrote: Understanding "-or" better than understanding "-o" is not a factor in this case, IMHO.

"Understanding" is nice; "correct functionality" is better. So the question is, which operators (POSIX ones or not) are better for this context where it has to work under Linux and Windows.

Potential scenario: several years from now someone suggests updating the auxiliary unxutils programs in UBCD because of some new need (or bug found, or whatever). Some MSYS tools are replaced by their CYGWIN counterparts, and the other way around (again, for some new needs, or customizations, or whichever other reason).

Giving this potential scenario, which operators (POSIX ones or not) would be more suited? I don't care if this script is usable "as-is" under, say, Solaris, or under any OS other than Linux and Windows.

So, I'm really asking. If "-o" is better suited for UBCD, let's use that. If "-or" is, then let's use it.
Ady sent me a PM about his "doubts" about these operators and want me to answer it.
First, read this. You can see that in POSIX only the '-o' operator is defined.
Next, let me quote a paragraph in the man page of GNU find:
expr1 -o expr2
Or; expr2 is not evaluated if expr1 is true.

expr1 -or expr2
Same as expr1 -o expr2, but not POSIX compliant.
It clearly states that '-or' is not POSIX compliant, but GNU supports both operators. That's why if you are more concerned about portability, using '-o' is better.

Does that answer ady's question?