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

Allow c-style comments in global.json #257

Closed
wants to merge 1 commit into from
Closed

Allow c-style comments in global.json #257

wants to merge 1 commit into from

Conversation

dorssel
Copy link

@dorssel dorssel commented Dec 6, 2021

Description:
Allow c-style comments in global.json.

Related issue:
Implements #254

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

@dorssel
Copy link
Author

dorssel commented Dec 6, 2021

Initial commit was from Windows: lots of EOL trouble + differences in index.js.
Force pushed from a Linux checkout + rebuild (should solve most of the problems).

@dorssel
Copy link
Author

dorssel commented Dec 20, 2021

@vsafonkin
Building fails with

Errors:
* setup-dotnet.npm.@actions/http-client
  filename: /home/runner/work/setup-dotnet/setup-dotnet/.licenses/npm/@actions/http-client.dep.yml
    - cached dependency record not found

* setup-dotnet.npm.semver
  filename: /home/runner/work/setup-dotnet/setup-dotnet/.licenses/npm/semver.dep.yml
    - cached dependency record not found

I don't know how to fix that. My local licensed gives no errors. I guess it has to do with the specific GitHub build environment/actions that I don't have locally?

@dorssel
Copy link
Author

dorssel commented Dec 20, 2021

http-client.dep.yml was split into 2 separate versions with the last merge on master: 1e92d49. Now licensed seems to expect a single version again.

I have no idea where semver.dep.yml comes from. I certainly did not add it.

@dorssel
Copy link
Author

dorssel commented Dec 20, 2021

New info: it is the npm version. GitHub build environment has updated and now uses npm 8.1.2. This changes the behavior of licensed a lot, including adding the ones that are missing in my checks but also removing a few dozen others.

This means the current master probably will not even build itself. I don't think fixing this should be part of this pull request. Can you fix master so we can all rebase our failing pull requests on a new and working baseline?

This is also the problem with PR #250.

@vsafonkin
Copy link

Hi @dorssel, the issue with license was fixed #258 , could you update your branch from main?

@dorssel
Copy link
Author

dorssel commented Dec 23, 2021

@vsafonkin Done

@xt0rted
Copy link

xt0rted commented Mar 7, 2022

Just ran into this issue in a new repo, any chance of getting this merged soon?

@dorssel
Copy link
Author

dorssel commented Mar 10, 2022

@xt0rted
This PR has been ready for merge for more than two months now. But I have no write access to this repo. And by now, there are merge conflicts which I cannot resolve, again due to missing write access. I have no clue how the workflow is for this project.

EDIT: I force-pushed after a rebase onto main. Should be mergable again.

@dorssel
Copy link
Author

dorssel commented May 11, 2022

.NET SDK 6.0.300 no longer allows comments in global.json either (although that may not be intended).
Nevertheless, this PR is stale. Not bothering to resolve merge conflicts (again...).

@dorssel dorssel closed this May 11, 2022
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