-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fixes #4405 About publishing nugets to public feed #4406
Conversation
@@ -174,13 +174,13 @@ phases: | |||
variables: | |||
BuildConfig: Release | |||
OfficialBuildId: $(BUILD.BUILDNUMBER) | |||
DOTNET_CLI_TELEMETRY_OPTOUT: 1 | |||
DOTNET_CLI_TELEMETRY_OUTPUT: 1 |
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.
@safern told me this was a typo that needed to be fixed. I believe this change doesn't affect directly the issue that is being fixed in this PR.
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.
Changing this was a mistake. If you search for DOTNET_CLI_TELEMETRY_OUTPUT
in all of github, you will find this is the only place this string is used. No one reads an OUTPUT
environment variable.
But if you search for DOTNET_CLI_TELEMETRY_OPTOUT
you will see it is used in hundreds of places.
Note: OPTOUT
stands for "opt out" - as in "don't log CLI telemetry".
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.
You’re completely right I really got confused by reading output and just seems wrong and I recommended to change, my bad @antoniovs1029
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.
packagesToPush: $(Build.SourcesDirectory)/bin/packages/**/*.nupkg;!$(Build.SourcesDirectory)/bin/packages/**/*.symbols.nupkg | ||
nuGetFeedType: internal | ||
feedPublish: MachineLearning | ||
|
||
# - task: MSBuild@1 |
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.
@codemzs I wonder if I should delete this commented task, since I believe this won't be used in the future?
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.
Yes, please remove.
@@ -226,21 +226,20 @@ phases: | |||
msbuildVersion: 15.0 | |||
continueOnError: false | |||
|
|||
- task: NuGetCommand@2 |
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.
I asked @codemzs and he told me he was okay with me removing this task that publishes the nugets into the private feed. Still, he was concerned about not knowing if this would "break any dependencies". It seems it doesn't, but if anyone has a reason for not removing this task, please tell me. (@eerhardt ?).
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.
So after I mentioned to @codemzs that publishing to the public feed might not always automatically work because of problems on the side of AzureDevOps, he recommended me to put back the task that publishes to the private feed.
Still, I decided to put it back after the other publishing tasks, since if the task of publishing to the private feed fails it would interrupt the other publishing tasks.
If you want to see it before the task of publishing to the public feed, please tell me.
Notice that having both tasks (publishing both to the public and to the private feed) increases the chances of having a failure on Azure DevOps side, making it more likely for the pipeline to fail.
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.
So after I mentioned to @codemzs that publishing to the public feed might not always automatically work because of problems on the side of AzureDevOps
This is not publishing to the public feed itself, the build definition already has that flakiness because of network connection issues or other unrelated issues to publishing... I think we shouldn't be publishing to both sides as it just slows down builds, it's expensive and nobody will be looking at the internal feed.
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.
I agree that the problem isn't in the publishing step itself. And I also agree that maybe it's better to not have the step that publishes to the private feed.
I've looked into the builds that were executed in the private pipelines before this PR, and it seems to me that in many cases the nugets weren't even published to the private feed because something went wrong with the pipeline.
After this PR got merged, and after Maryam's latest PR got merged an hour later, their respective automatic builds failed. I've also tried to manually rerun the builds, but they also fail. The failures occur in steps of the pipeline that execute even before trying to publish to the public feed, so this failures are not related to the changes I added in this PR; for example, they fail while installing a plug in, or while signing builds.
So, so far, the things I've introduced in this PR haven't been actually executed in Azure DevOps in the master branch (I've only tested my changes by running them from a feature branch in the private repo, and that worked as expected and published the nugets to the feed, but even then it required me to run a couple of manual builds because of the same kind of problems I've already described).
I am somewhat worried about all of these, because it seems that other people will rely on the public feed to be always up-to-date (like @frank-dong-ms nightly build tests). But I wouldn't know if there could be other options, or workarounds to all of this... or if this actually poses a problem to them.
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.
Ok, so in the last two builds (one manual, and one triggered automatically after @codemzs latest PR), the task of publishing to the public feed ran successfully, but the pipeline actually failed when publishing to the private feed (and the errors in both cases were different, so my guess is that this was, again, caused by an error on Azure Devops side).
So at least the changes I've made in this PR seem to be working for publishing to the public feed. It's just that it might require some luck for the whole pipeline to run without problems.
Notice, though, that even if the pipeline says that the nugets were published successfully to the public feed after Zeeshan's PR got merged 3 hours ago, if I check the public feed the nugets were last updated 5 hours ago (after a manual build I ran with the master branch before Zeeshan's PR). @safern had told me yesterday that sometimes it gets some time (even over an hour) for the Feed to actually get updated, so maybe this is also something that other people relying on the public feed should be aware of.
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.
About that last point, it actually seems that the nugets were correctly published to the feed, but the feed decides (for whatever reason) to select previous published nugets and show them directly in the feed instead of the latest ones. If I go to the 'versions' tab of a specific nugget, I actually can see that it got updated on the last builds, but it is selecting to show the one built 8 hours ago:
https://dev.azure.com/dnceng/public/_packaging?_a=package&feed=MachineLearning&package=Microsoft.Extensions.ML&protocolType=NuGet&version=1.4.0-preview3-28229-9&view=versions
I am guessing this is, for some reason, the expected behavior, and that I was wrong when saying that the Feed takes time to reflect the changes... it just doesn't display the latest version on the feed.
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.
Let's only have a single feed - the public feed. Having 2 feeds will only increase confusion, storage, and increase the chance of failure.
@@ -257,6 +256,14 @@ phases: | |||
msbuildVersion: 15.0 | |||
continueOnError: true | |||
|
|||
- task: NuGetCommand@2 |
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.
As I mentioned here, I don't think it is worth keeping this.
* Publish nugets to azure devops public feed
* Publish nugets to azure devops public feed
Fixes #4405 about publishing nugets to this public feed:
https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=MachineLearning
This will be executed by the AzureDevOps build pipeline whenever a new commit is added to the master branch of this repo. Notice that sometimes there are some problems on the side of Azure DevOps, and it might fail when executing the build pipeline, even in steps that were not modified in this PR, and producing errors that prevent the pipeline to actually publish the nugets to the feed; this unplanned errors already existed before the changes introduced in this PR, and they are somewhat unpredictable. The solution to this is to rerun the build manually until the pipeline succeeds.
Worked this out following @safern instructions.