Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DYN-5944 Removed NDesk.Options Library #14106
DYN-5944 Removed NDesk.Options Library #14106
Changes from 1 commit
cb3e0ea
0f9174d
4bb7b70
d0b8b8d
91fafcd
2fb22c2
61b2630
f1e6aec
7cdc64c
e855f59
324bb3e
fc1a09f
2e0af94
33a1310
de6659d
4e5216a
fc78c58
c819987
8e44108
ad9c4ef
2cf6b72
bda2f98
3d73767
4d6b5cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need this to run on net48 too. Please verify if the new way works for net48. If not you will have to bring back the old way with (ilmerge) under the net48 condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 As I told you before, this was built it correctly on .NET 4.8 and also I built it on .NET 6, and there wasn't any error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JimmySanchezGlobant please see the comment above from the bin diff job - the exe is not being produced:
#14106 (comment), so even if there are no errors, something seems off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JimmySanchezGlobant the new flags do not work for net48 target.
You need to use the old way (with ILMerge) for net48
And the new flags + Add a new publish target (the flags work on publish only I think)
ex
But this should be added only under net6 target somehow. Also
SelfContained
should be set to falseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 The old way with ILMerge is failing, when I build the project it's returning the error code (missing file), so, it's not clear to me how this can solve this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ILMerge should only work under net48. If you try to build from master it still works right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 Yes, it's working well under . Net 4.8 from the master branch, it's weird, yesterday I did exactly the same test and it was failing on the ILMerge command, so, I don't know what to do to solve this, to be honest, I'm still very confused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pinzart90 ... two thoughts - I don't see that this PR updates any of the tests. I think that ndesk options and command line parser require setting flags differently... ie
/
vs-
or--
, I can't remember for certain.Have you tried running the docs browser tests locally? Perhaps the markdown converter tool is never shutting down causing some kind of deadlock in the test runner etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjkkirschner good catch, the md2html tests do seem to hang locally. Hopefully it is the same cause on the build machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess it's also possible that this is somehow messing with Console.Out or modifying the input argument list so it never makes its way to the rest of the Main function?