Skip to content

Refactor uninstall-script generation #1380

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

Merged
merged 10 commits into from
Aug 16, 2023

Conversation

vector-of-bool
Copy link
Contributor

This aside change refactors and changes a few minor issues with the uninstall script:

  1. The script did not handle a late-defined install-prefix, and entered an infinite loop in that case
  2. The script handled DESTDIR strangely (?)
  3. There were two different generators: for Windows and for Unix
  4. Using .bat scripting at all is just fraught with peril and should be avoided, if at all possible

This change also removes the top-level generate_uninstall/ directory, as well as the two generator scripts, instead replacing it with a single GenerateUninstalle.cmaker module (which should also be portable between projects).

Copy link
Contributor

@rcsanchez97 rcsanchez97 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 a little sad that you didn't like my generator script written in batch 😞

Seriously, though, this is much nicer. Aside from a few minor tweaks/questions, it looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming that the DEBUG messages were meant to be left in. I don't have a problem with them (I rather like the extra output), but I wanted to mention it in the event you intended to remove them and forgot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they were useful while debugging and don't get in the way, so I just left them there.

)

# If applicable, generate an "uninstall" target to run the uninstaller:
if(CMAKE_SOURCE_DIR STREQUAL PROJECT_SOURCE_DIR OR PROJECT_IS_TOP_LEVEL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how a collision is prevented between the uninstall generated in this project and, for instance, the uninstall generated in the C++ driver when it includes the C driver directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is mostly the same as what was done previously, but with the addition of the newer CMake feature PROJECT_IS_TOP_LEVEL, which was added to address this exact use case (on older CMake, that variable check will be a no-op).

Co-authored-by: Roberto C. Sánchez <[email protected]>
@vector-of-bool vector-of-bool merged commit 070b35a into mongodb:master Aug 16, 2023
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