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

lower the build requirements #517

Merged
merged 1 commit into from
Nov 4, 2023

Conversation

xambroz
Copy link
Contributor

@xambroz xambroz commented Nov 3, 2023

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

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

PartialVolume commented Nov 3, 2023

@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
Copy link
Contributor Author

xambroz commented Nov 4, 2023

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
Copy link
Collaborator

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
Copy link
Contributor

ggruber commented Nov 4, 2023

no particular reason, it happend just as @xambroz assumed

@ggruber
Copy link
Contributor

ggruber commented Nov 4, 2023

@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
Copy link
Collaborator

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
Copy link
Contributor

ggruber commented Nov 4, 2023

ok. start working right now :-)

@ggruber
Copy link
Contributor

ggruber commented Nov 4, 2023

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
Copy link
Contributor

ggruber commented Nov 4, 2023

progress dots are in PR

@ggruber
Copy link
Contributor

ggruber commented Nov 4, 2023

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
Copy link
Contributor

ggruber commented Nov 4, 2023

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
Copy link
Contributor Author

xambroz commented Nov 4, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

PartialVolume commented Nov 4, 2023

@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
Copy link
Collaborator

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
Copy link
Contributor

ggruber commented Nov 4, 2023

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
Copy link
Contributor

ggruber commented Nov 4, 2023

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
Copy link
Contributor

ggruber commented Nov 4, 2023

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

@PartialVolume PartialVolume merged commit 4bf06d1 into martijnvanbrummelen:master Nov 4, 2023
2 checks passed
@ggruber
Copy link
Contributor

ggruber commented Nov 5, 2023

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

@PartialVolume
Copy link
Collaborator

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?

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