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

Add AMPL package #23974

Closed
wants to merge 10 commits into from
Closed

Add AMPL package #23974

wants to merge 10 commits into from

Conversation

robgics
Copy link
Contributor

@robgics robgics commented May 27, 2021

No description provided.

tgamblin
tgamblin previously approved these changes May 28, 2021
@tgamblin
Copy link
Member

This is failing style checks.

@alalazo alalazo self-assigned this Jun 1, 2021
@alalazo
Copy link
Member

alalazo commented Jun 1, 2021

@robgics Currently there's this style issue:

==> Error: Flake8 style checks found errors
var/spack/repos/builtin/packages/ampl/package.py:59: [W391] blank line at end of file

Feel free to ping me when solved.

@robgics
Copy link
Contributor Author

robgics commented Jun 2, 2021

How do I ping you? I should have run the style test locally, as that seems to work (though unit-tests don't). I fixed the extra lines. I've never pull-requested before...do I just commit my changes and "resubmit" this pull request?

@robgics
Copy link
Contributor Author

robgics commented Jun 2, 2021

Ok, I see that committing automatically updates the pull request, and that it failed again. It's complaining about a license thing. I take it that's not part of "style", because the style test did not complain about that when I ran locally.

@robgics
Copy link
Contributor Author

robgics commented Jun 2, 2021

Ok, I admit, I can't figure out what the "expected license" is that Spack wants.

Comment on lines 1 to 3
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
# SPDX-License-Identifier: (Apache-2.0 OR MIT)
Copy link
Member

Choose a reason for hiding this comment

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

All the packages start with:

Suggested change
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
# SPDX-License-Identifier: (Apache-2.0 OR MIT)
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

Please restore the extra line 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it because it was still giving me the general license error and I was trying to match it with what was in cmd/license.py. Once I removed it, I've now passed the tests. I will add the line back in and commit again.

@robgics
Copy link
Contributor Author

robgics commented Jun 3, 2021

If this works, I need to make this change in the branch I have created for this, not in "develop".

@robgics robgics closed this Jun 3, 2021
@robgics
Copy link
Contributor Author

robgics commented Jun 3, 2021

Doing this according to the contrib guide, in a specific branch.

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

Successfully merging this pull request may close these issues.

None yet

3 participants