Skip to content

Fix make distclean#10895

Closed
bernd-edlinger wants to merge 3 commits intoopenssl:masterfrom
bernd-edlinger:fix_make_distclean
Closed

Fix make distclean#10895
bernd-edlinger wants to merge 3 commits intoopenssl:masterfrom
bernd-edlinger:fix_make_distclean

Conversation

@bernd-edlinger
Copy link
Copy Markdown
Member

This makes distclean work correctly in master.
The windows issue is complicated by two effects,
which may be due to the specific windows machine.
First it has one of the commands to delete all generated targets
exceed the command line limit of windows.
And this machine has a cygwin somewhere in the path
almost at the end but its /usr/bin/rmdir nevetheless
overrides the builtin rmdir which makes the rmdir fail,
because it does not recognize "/S" and "/Q".

After make distclean the following directories are
still around:

 crypto/include
 crypto/providers
 doc/man
 doc/html

This patch cleans those up or avoids to create them in
the first place.

[extended tests]
First the line "del /Q /F $(GENERATED)" was too long
then it happens that nmake dies not invoke the builtin
rmdir if a cygwin/bin is in the path.
Avoid both by using perl for removing files and dirs.

[extended tests]
@bernd-edlinger bernd-edlinger added the branch: master Applies to master branch label Jan 19, 2020
@bernd-edlinger
Copy link
Copy Markdown
Member Author

I would suggest to cherry-pick the second commit "Remove pod2htmd.tmp in make clean"
to 1.1.1.

I have not observed the overlong line in windows make clean, for 1.1.1,
but instead of pod2htmd.tmp there are two of them: pod2htmd.tmp and pod2htmi.tmp
and the rmdir does not work if any cygwin is around.

Copy link
Copy Markdown
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

I'm okay with the changes but don't understand the windows-makefile.tmpl ones well enough.
@levitte perhaps?

@bernd-edlinger
Copy link
Copy Markdown
Member Author

bernd-edlinger commented Jan 23, 2020

Richard, you probably remember the issue with the "echo *" on windows, which
which was supposed to just print "*" in this case, but that causes a surprise
when cygwin's echo command is also in the path. then "*" is expanded as if it
was a linux echo. You wrote an echo.pl to fix that issue.
Now the rmdir is the same issue, I see no way how to make nmake use the builtin.
The other issue I had with windows is a command length in del $(GENERATED)
that happened only on master, 1.1.1 did not exceed the line length fortunately.
I saw no other solution than to use perl for that and use a temp file to pass the list.

Copy link
Copy Markdown

@vdukhovni vdukhovni left a comment

Choose a reason for hiding this comment

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

Looks OK to me, but this requires testing on platforms I don't have access to, so I don't feel I can give the final thumbs up.

@bernd-edlinger
Copy link
Copy Markdown
Member Author

I did test that on a windows 7 machine.
I use remote access to the our internal company network.
I can still do that, but I consider this not necessary.

@bernd-edlinger
Copy link
Copy Markdown
Member Author

so I don't feel I can give the final thumbs up.

Maybe you just trust me then?

@vdukhovni
Copy link
Copy Markdown

so I don't feel I can give the final thumbs up.

Maybe you just trust me then?

I do, but if Richard is free to take a look, he's much more familiar with the build-system than I. I am not sure I be doing approvals after only cursory inspection. Though perhaps with the build-system I may not need to be quite so cautious. Ping me again in a couple of days if nobody else steps forward...

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Mar 28, 2020

so I don't feel I can give the final thumbs up.

Maybe you just trust me then?

You know it quite well, reviewing is not a matter of trust or distrust. It‘s simply better if someone else checks the changes independently. As the proverb says: „Vertrauen ist gut, Kontrolle ist besser.“

Maybe I find some time to test it...

@bernd-edlinger
Copy link
Copy Markdown
Member Author

bernd-edlinger commented Mar 28, 2020

Just for reference, when I post a GCC patch,
I write in the e-mail

Bootstrapped and Reg-tested on x86_64-pc-linux-gnu
Is it OK for thunk?

bootstrap takes hours, and the test suite a day or more.
every reviewer trusts me, that I say that honestly,
the review is about how the code looks like nothing more nothing less.
If they start to not trust a person that is listed in "Write after Approval"
they have to duplicate all my effort, and moreover there is
no CI, it would not be usable since the test suite that runs for days.
Just automated test runs after a patch is applied.

@bernd-edlinger
Copy link
Copy Markdown
Member Author

Removing windows stuff for now.
I don't know if the nmake distclean was fixed by @levitte
but I dont care about windows any more.
In the moment I see the following files are leaking again,

$ git clean -ndx
Would remove crypto/include/
Would remove providers/fipsinstall.cnf
Would remove test-runs/

So since I started this PR new bugs were introduced,
nobody reviewed this, for a long time, if I spend time
to fix this PR again, it will not be reviewed again,
probably I better give up right now before I waste more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants