martijnvanbrummelen / nwipe

nwipe secure disk eraser
GNU General Public License v2.0
682 stars 77 forks source link

lower the build requirements #517

Closed xambroz closed 9 months ago

xambroz commented 9 months ago

Hello, please lower the requirements to autoconf allowing to build rhel7/8 packages (possibly rhel6, not tested). Thank you Michal Ambroz

PartialVolume commented 9 months ago

@ggruber Before I lower the build requirements back to 2.63 for RHEL, I just want to check with you whether there was a particular reason you changed them to 2.71? (October 1st commit).

Was AC_PREREQ([2.64]), now AC_PREREQ([2.71])

xambroz commented 9 months ago

I guess just the autoupdate was executed on a system with version 2.71. I believe the tool is not putting the real minimum requirement, but just the current version. It rebuilds with 2.69 just fine. Version 0.34 was building well with 2.63/rhel6).

Here is the copr build for Fedora 37-40 , RHEL 8/9 with this patch: https://copr.fedorainfracloud.org/coprs/rebus/infosec/build/6596188/

RHEL8 succeeded thanks to this (autoconf 2.69), would be failing otherwise. RHEL7 is failing from some other reason .

PartialVolume commented 9 months ago

As far as Debian builds are concerned I don't see any problem with reverting back to 2.63 or 2.64. I think autogen.sh would complain about something (which I'll check later) but otherwise the build would work fine.

Just waiting to hear back from @ggruber as I'm not sure what distro he's building on and whether it was changed to 2.71 because it was preventing a build or like Debian was just a warning. If just a warning then I'll commit the change back to 2.63

https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Versioning.html

ggruber commented 9 months ago

no particular reason, it happend just as @xambroz assumed

ggruber commented 9 months ago

@PartialVolume btw, if it would help: on the R720XD Proxmox is installed. Just on the boot mirror we could have a few distros as kind of build farm And I also thought about having a gitlab locally, so all the CI/CD stuff could be done here.

PartialVolume commented 9 months ago

no particular reason, it happend just as @xambroz assumed

@ggruber Ok, that's good. I think I'll delete the current 0.35 release and commit the suggested changes.

If you want to make those fine tuning changes, i.e progress dots you wanted to make now would be the opportunity before I re-release 0.35. If that's something you still want to do?

Using the server with multiple distros for build testing sounds like a good idea. 👍

ggruber commented 9 months ago

ok. start working right now :-)

ggruber commented 9 months ago

just as I saw this: device.c, ~line 138

    /* Check whether this drive is on the excluded drive list ? */
    idx = 0;
    while( idx < 10 )
    {

where does this fixed "10" come from?

ggruber commented 9 months ago

progress dots are in PR

ggruber commented 9 months ago

Using the server with multiple distros for build testing sounds like a good idea. 👍

Which distros should we have? debian 10,11,12? ubuntu 20.04LTS, 22.04LTS, 23.10 RHEL8, 9 (Rocky 8,9) CentOS7/RHEL7 SLES?

ggruber commented 9 months ago

btw, have an interesting hardware @work currently, with 50 disks to wipe 2023-11-04 14_10_31-manyDisks as most are connected dual ported in a JBOD shelf they appear twice this box is the reason to ask for the progress dots as these 3TB "SAS" disks are as slow as the HPs in my R720

xambroz commented 9 months ago

please wait with the 0.35 re-release ... I am testing another patch, order of includes in gui.c is preventing build on older systems like rhel6/7

PartialVolume commented 9 months ago

please wait with the 0.35 re-release ... I am testing another patch, order of includes in gui.c is preventing build on older systems like rhel6/7

Ok, there's no rush. Let me know when you are ready. I'm not expecting to re-release this weekend, most likely early next week if everybody is happy.

PartialVolume commented 9 months ago

@ggruber

just as I saw this: device.c, ~line 138

    /* Check whether this drive is on the excluded drive list ? */
    idx = 0;
    while( idx < 10 )
    {

where does this fixed "10" come from?

That 10 needs to be replaced with MAX_NUMBER_EXCLUDED_DRIVES. The define already exists in options.h. We currently allow a max of 10 drives to appear in the excluded drives list.

See line 410 options.c

line 410 while( optarg[idx_optarg] != 0 && idx_drive < MAX_NUMBER_EXCLUDED_DRIVES )

So line 138 in device.c can be changed to

/* Check whether this drive is on the excluded drive list ? */
>     idx = 0;
>     while( idx < MAX_NUMBER_EXCLUDED_DRIVES )
>     {

Ok, if you make the change? If not I can do it.

PartialVolume commented 9 months ago

Using the server with multiple distros for build testing sounds like a good idea. 👍

Which distros should we have? debian 10,11,12? ubuntu 20.04LTS, 22.04LTS, 23.10 RHEL8, 9 (Rocky 8,9) CentOS7/RHEL7 SLES?

I don't think we need to test on all of Debian 10, 11 & 12, just on the latest Debian SID, unless @martijnvanbrummelen says otherwise. If you take a look at the Repology list maybe that might help figure out which versions of distro nwipe gets ported to. Having said that there maybe LTS releases that some people want to build nwipe on.

I guess if a automated build script can be written so that with a single command it builds on all the distros we have chosen then maybe the more the merrier.

Is that something you could setup on your server?

ggruber commented 9 months ago

sry for the delayed answer, missed having a look at my mails

MAX_NUMBER_EXCLUDED_DRIVES

why this number of 10 was choosen? To have not 26, 32 or even 100 instead should noadays make no difference, we should have enough RAM in either case.

well, on the other hand: in 97% of all use cases of nwipe 10 should be sufficient. But we should abort if from whatever reason more disks a given on cli. Have to check this in the code, otherwise I'd tend to increase MAX_NUMBER_EXCLUDED_DRIVES to 32.

ggruber commented 9 months ago

PR is there. A silent ignore of parameters exceeding the MAX_NUMBER_EXCLUDED_DRIVES as in the code before would result in wiping disk that should be excluded. I would not like such a situation, so I increased the value in options.h to 32. besides the handling of the exceeding.

ggruber commented 9 months ago

regarding the distros we could set up a buildservice for: if we do this we could also offer repos for the currently supported enterprise editions, which was the idea behind the list from my above post. on the other hand, disk wipe systems or often started from a stick or CD image. mmh, not sure, how to make the current version with it's enormous enhancements available for a broad crowd of admins interested in such a tool

ggruber commented 9 months ago

another PR will follow with a slightly modified sorting (function) ... PR pending

PartialVolume commented 9 months ago

I think I'd be looking at why a trip temperature of 40 Deg.C is being reported. Is that trip temperature a valid trip temperature as specified in the manufacturer's drive specs?

If it's actual trip temperature is 45 Deg.C as reported by the drive and verified by the drive specification and only a single temperature is provided by the drive then that's why we shouldn't artificially substitute values, i.e adding the -5 degree value I think it was in the code. It's unnecessary as the GUI code that handles the colour of the temperature will quite happily work with a single limit.

If that's the case the arbitrary -5 that gets substituted in the code should just be removed and the variable left in its original initialised state of 10000000 or whatever the number was.

I don't think you need a force option, it's not like nwipe is restricting or slowing I/O to the drive, the temperature colour is just a benign warning that can be ignored.

I would remove that arbitrary -5 though if those drives genuinely have a 45 Deg.C temperature operating limit according to the drive specs. What make model are those drives?