Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move compile-mbr.sh script into fail-mbr's automake #255

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Nov 2, 2024

Calling bare tools like gcc is not best practise, as this will fail when cross-compiling. We also don't need to rely on tools like rpm when the build already knows which CPU we're building for.

Cross-compiling is still broken for other reasons, but that can be addressed later.

Calling bare tools like "gcc" is not best practise, as this will fail
when cross-compiling. We also don't need to rely on tools like rpm when
the build already knows which CPU we're building for.
@Thomas-Tsai Thomas-Tsai merged commit 95527c2 into Thomas-Tsai:master Dec 16, 2024
1 check passed
Comment on lines -13 to -20
# compile the file fail-mbr.bin
#echo -n "Compiling: fail-mbr.S -> fail-mbr.o -> "
#gcc -Wall -Werror -m32 -nostdlib -static -Wl,--relocatable -o fail-mbr.o fail-mbr.S

#echo -n "fail-mbr.image -> "
#gcc -Os -Wall -W -Wshadow -Wpointer-arith -Wmissing-prototypes -Wundef -Wstrict-prototypes -g -falign-jumps=1 -falign-loops=1 -falign-functions=1 -mno-mmx -mno-sse -mno-sse2 -mno-3dnow -fno-dwarf2-cfi-asm -fno-asynchronous-unwind-tables -m32 -fno-stack-protector -mno-stack-arg-probe -Werror -Wno-trampolines -DUSE_ASCII_FAILBACK=1 -DHAVE_UNIFONT_WIDTHSPEC=1 -mrtd -mregparm=3 -fno-builtin -m32 -Wl,--build-id=none -nostdlib -Wl,-N,-S -Wl,-N -Wl,-Ttext,0x7C00 -Wl,--no-dynamic-linker -o fail-mbr.image fail-mbr.o

#echo "fail-mbr.bin [Done]. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we loose this? These steps are important to know an have, if we are in a similar situation like at #211 again…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it wasn't clear to me why this was commented out. I figured it was just unused. Please could you explain. We could add it back, perhaps as Makefile rules that don't get called automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's for targeting 32-bit on 64-bit, but there's only one fail-mbr.bin.orig to compare against?

Copy link
Contributor

@robert-scheck robert-scheck Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not an expert at this. I only can tell that the commented steps helped me to track down the difference step by step when using older and newer tools.

Maybe just keep them in a comment for possible future comparison/debugging activities?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting aside the suggestion that this can be dropped, I do find it a little surprising that you can seemingly always get the same result as fail-mbr.bin.orig without all those extra flags. Since that does seem to work, you can just run configure followed by make -C fail-mbr fail-mbr.bin, which will simply build it for you. If you need to tweak the flags, just copy the output and adjust or edit Makefile.am as required.

@chewi chewi deleted the fail-mbr-automake branch December 17, 2024 21:14
@Thomas-Tsai
Copy link
Owner

Thank you for your discussions and contributions. This opportunity allowed us to discuss the usage scenarios of the failmbr program, which you can refer to here.
The failmbr is rarely used and we consider it should be removed from partclone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants